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

CRM-21733: Adding the ability to override membership until specific date #11622

Merged
merged 1 commit into from
Feb 14, 2018
Merged

CRM-21733: Adding the ability to override membership until specific date #11622

merged 1 commit into from
Feb 14, 2018

Conversation

omarabuhussein
Copy link
Member

@omarabuhussein omarabuhussein commented Feb 2, 2018

Overview

Improving membership status override to provide extra options to allow temporary status override.

Before

If you want to override a membership status, there is a checkbox field called (Status Override?) that you can check/uncheck.

error

After

Instead of having a checkbox called (Override Status?) in membership add/edit form, it will be replaced with a select box that allow the user to choose one of three options :

1- No
2- Override Permanently
3- Override until selected date

If the first option is selected, then the membership will behave as if the old (Override Status?) is unchecked, which means that the membership is subject to membership status rules.

If the 2nd option is selected, then the membership will behave as if the old (Override Status?) is checked, which mean that the membership status is overridden and is not subject to the membership statues rules.

If the 3rd option is selected, then a new field will appear allowing the user to choose a date, in this status, the membership will behave similar to option 2, but when today date is equal or less than the selected date, then the "Update Membership Statuses" scheduled job will automatically convert its status back to No, which means that the membership status is overridden temporarily only for the selected date and after that it will revert back and be subject to membership status rules

after

Technical Details

A new field 'status_override_end_date' is added to the membership entitiy where its default value is NULL, if the value of this field is set to certain date then the membership will be considered 'Temporarily' until that date.

Most parts of CiviCRM will still use is_override as is and nothing much changed, most notable parts that will be aware of these new changes are :

  • The membership form controller : where a selected box is now showing one of three values where is not overridden = 0, is overridden = 1 and overridden until date = 2, if either 1 or 2 is selected then is_override will be set to True or false otherwise.

  • "Update Membership Statuses" scheduled job : which will check any field with is_override and status_override_end_date set and set is_override to False and status_override_end_date to Null if the membership status override expired , hence that if the membership status override end date = today date then it will be considered expired.

@@ -2279,6 +2280,8 @@ public static function updateAllMembershipStatus() {
continue;
}

self::processOverridenUntilDateMembership($dao);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

processOverridenUntilDateMembership -> processOverriddenUntilDateMembership

* @param CRM_Member_DAO_Membership $membership
* The membership to be processed
*/
private static function processOverridenUntilDateMembership($membership) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

processOverridenUntilDateMembership -> processOverriddenUntilDateMembership

@mattwire
Copy link
Contributor

mattwire commented Feb 2, 2018

@omarabuhussein This looks good on a quick review - I haven't actually tested it yet. I think we'll need to add unit tests to provide coverage for this functionality (and the existing override functionality) though I think there are probably tests in api/v3/MembershipTest and CRM/BAO/MembershipTest that you could build on.

@eileenmcnaughton
Copy link
Contributor

thanks @omarabuhussein that looks much less scary - & @mattwire comments above are pretty much the same comments I would have made

@omarabuhussein
Copy link
Member Author

haha, thanks @eileenmcnaughton and @mattwire , I've added tests and fixed some other stuff then squashed the commits. I just need to update the PR description to provide more details.

@omarabuhussein
Copy link
Member Author

omarabuhussein commented Feb 2, 2018

I see that civicrm.mysql is ignored in gitignore file, my memory is a bit vague about it, it will be regenerated and updated automatically before the release, right ?

@omarabuhussein omarabuhussein changed the title (WIP) CRM-21733: Adding the ability to override membership until specific date CRM-21733: Adding the ability to override membership until specific date Feb 3, 2018
@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Feb 12, 2018

I took a look & I'm happy the code looks cleen & the test coverage is good and this should now have minimal impact on existing flows (I can't see anything that would be a breaking change.). I UI tested & it worked except from when changing from a temporary override to permanent - it did not null out the override date.

civicrm.mysql is ignored but there should be another generated sql file change I believe

@eileenmcnaughton
Copy link
Contributor

Since 4.7.31 is now cut & the upgrade script against this is on 4.7.32 I'm going to merge it now. Per above comments my concern was that I think not all the generated assets are in here - but I will do that as a follow up

@eileenmcnaughton eileenmcnaughton merged commit c9dfdcd into civicrm:master Feb 14, 2018
@omarabuhussein
Copy link
Member Author

thanks a lot @eileenmcnaughton

I UI tested & it worked except from when changing from a temporary override to permanent - it did not null out the override date.

Ops! This is important for the cron job, I have to fix it, will do it soon.

is ignored but there should be another generated sql file change I believe
Since 4.7.31 is now cut & the upgrade script against this is on 4.7.32 I'm going to merge it now. Per above comments my concern was that I think not all the generated assets are in here - but I will do that as a follow up

hmm thanks, but I am not really sure which files should be regenerated, the process is not documented anywhere I believe. I can do it togther with the above fix if you have an idea ?

@eileenmcnaughton
Copy link
Contributor

I re-ran civibuild create after merge & no file changes & all seems good so I guess there isn't another file to fix

@eileenmcnaughton
Copy link
Contributor

@omarabuhussein FYI #12563

@omarabuhussein omarabuhussein deleted the CRM-21733-membership-override-improvments branch July 27, 2018 19:06
@omarabuhussein
Copy link
Member Author

thanks @eileenmcnaughton , you were faster than me to fix the notice issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants