Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix period_type, duration_unit, duration_frequency to be required on membership type form #13227

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Dec 4, 2018

Overview

Fixes a regression where some required fields are not showing as required

Before

Duration unit, frequency & plan type (period_type) not showing as required

After

fields required

screenshot 2018-12-05 10 58 13

Technical Details

@mattwire this is from changes to the membership type form - I feel like we should get this merged for the rc (@monishdeb @seamuslee001 maybe one of you can merge).

I also think that MembershipType.xml should have these fields marked as required - I am always a bit hesitation to make changes like that in an update script in case a site has already some nulls & it causes pain. However, changing the schema only would change the DAO / new installs

Comments

@civibot
Copy link

civibot bot commented Dec 4, 2018

(Standard links)

@civibot civibot bot added the master label Dec 4, 2018
@eileenmcnaughton eileenmcnaughton changed the base branch from master to 5.8 December 4, 2018 22:27
@civibot civibot bot added 5.8 and removed master labels Dec 4, 2018
@seamuslee001
Copy link
Contributor

(CiviCRM Review Template WORD-1.2)

  • General standards
    • (r-explain) Pass
    • (r-user) Pass fixes issue where required fields weren't marked as such
    • (r-doc) Pass
    • (r-run) Pass confirmed the fields now show required where they are meant to be
  • Developer standards

@seamuslee001
Copy link
Contributor

Merging (don't need merge on pass as test build finished)

@seamuslee001 seamuslee001 merged commit ae586ee into civicrm:5.8 Dec 5, 2018
@seamuslee001 seamuslee001 deleted the membership_type branch December 5, 2018 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants