Remove inheritance of MembershipConfig form from MembershipStatus form. #12184
Conversation
eileenmcnaughton
force-pushed
the
entity_form
branch
from
8391751
to
f894ca0
Compare
| parent::buildQuickForm(); | ||
|
|
||
| if ($this->_action & CRM_Core_Action::DELETE) { | ||
| return; | ||
| } | ||
|
|
||
| $this->applyFilter('__ALL__', 'trim'); |
There was a problem hiding this comment.
in self::buildQuickEntityForm();
|
test this please |
| @@ -60,7 +60,8 @@ protected function setEntityFields() { | |||
| $this->entityFields = [ | |||
| 'label_a_b' => [ | |||
| 'name' => 'label_a_b', | |||
| 'description' => ts("Label for the relationship from Contact A to Contact B. EXAMPLE: Contact A is 'Parent of' Contact B.") | |||
| 'description' => ts("Label for the relationship from Contact A to Contact B. EXAMPLE: Contact A is 'Parent of' Contact B."), | |||
| 'required' => TRUE, | |||
There was a problem hiding this comment.
accidentally changed to non-mandatory in previous commit
There was a problem hiding this comment.
This bit should probably be in it's own PR as it's unrelated to the other changes
There was a problem hiding this comment.
RIght - it leverages the fact this PR adds the ability to set required. I'm OK to do that - I was just a bit scared that it could get lost - but if you feel the PR is otherwise mergeable I'll take it out
There was a problem hiding this comment.
Ok, leave me to review/test the rest of the PR then first :-)
There was a problem hiding this comment.
@mattwire Ok - I could pull that part out & do the required bit first & make this depend on that - either way but there is an interdependency
There was a problem hiding this comment.
Confirmed this change is correct. Let's leave it in the PR.
There was a problem hiding this comment.
We should add 'required' to the docblock above too * - required (Is this field required or not?)
There was a problem hiding this comment.
@eileenmcnaughton Tested and confirmed that everything is working as before. I'd like to see the delete message updated as it's not really reflecting what happens (though I realise it was like that before).
This is a good iterative improvement to the code that will help consistency and maintainability in the future, and subject to the delete message wording is good to merge.
CRM/Member/Form/MembershipStatus.php
Outdated
| * We do this from the constructor in order to do a translation. | ||
| */ | ||
| public function setDeleteMessage() { | ||
| $this->deleteMessage = ts('WARNING: Deleting this option will result in the loss of all membership records of this status.') . ' ' . ts('This may mean the loss of a substantial amount of data, and the action cannot be undone.') . ' ' . ts('Do you want to continue?'); |
There was a problem hiding this comment.
@eileenmcnaughton I realise that this is just a copy of the original message, but it's not clear to me what will happen - does it really mean that actual memberships will be deleted?
In fact, looking in CRM_Member_BAO_MembershipStatus::del() it would seem that a) it's only referring to the membershipstatus, and b) it won't delete anything if there are memberships with this status.
I've just confirmed that to be the case.
Something like:
"You will not be able to delete this membership status if there are existing memberships with this status. You will need to check all your membership status rules afterwards to ensure that a valid status will always be available. Please confirm that you wish to delete this status?"
| @@ -60,7 +60,8 @@ protected function setEntityFields() { | |||
| $this->entityFields = [ | |||
| 'label_a_b' => [ | |||
| 'name' => 'label_a_b', | |||
| 'description' => ts("Label for the relationship from Contact A to Contact B. EXAMPLE: Contact A is 'Parent of' Contact B.") | |||
| 'description' => ts("Label for the relationship from Contact A to Contact B. EXAMPLE: Contact A is 'Parent of' Contact B."), | |||
| 'required' => TRUE, | |||
There was a problem hiding this comment.
Confirmed this change is correct. Let's leave it in the PR.
This is part of trying to move towards an entity form (ie. support custom data) for MembershipType. Currently the 2 forms share a parent class - but really the add buttons is the only shared functionality. This change uses the EntityTrait to add the buttons as well. There is some minor tidy up in there but I tried to keep it very limited. I switched only one field over to being added via addField, the parent mechanism so far
eileenmcnaughton
force-pushed
the
entity_form
branch
from
487f872
to
a2fdee8
Compare
|
changes made per review - merge on pass |
Overview
Code tidy up on Membership Status form towards the general goal of supporting custom data on a range of forms
Before
MembershipType form & MembershipStatus form share a parent class, but very little of the code on that class is shared
After
MembershipStatus form directly extends CRM_Core_Form and implements the EntityForm trait to get the buttons. There is some minor tidy up in there but I tried to keep it very limited.
Technical Details
I only added 2 fields to the entity form mechanism.
Comments
@colemanw @mattwire this is a cleanup towards further entity form standardisation if one of you can look
@colemanw I realised I also needed to fix the RelationshipForm once & fix label being accidentally set to not required in previous changes - included here