MAE-398: Make showing/hiding events "same email address?" setting configurable #21

ahed-compucorp · 2020-12-15T09:29:25Z

Before

In CiviEvent Extras Settings page.

In "Online Registration" in event creation/editing you will see "Same email address?" field.

After

In CiviEvent Extras Settings page.

In "Online Registration" in event creation/editing you will not see "Same email address?" field.

Technical Details

My first commit refactored the old way which depends on saving the CSS class of the element in EventsExtras.setting.php. Now it is doing that as well as sets the default values inside the buildform hook completely.
My 2nd commit fixes a bug I found which results in not putting the default values for some of the "CiviEvent Extras" Settings like Those fields have missing default value: Currency, "Enable Pay Later option?", "Pay Later Label", "Pay Later Instructions", "Billing address required", "Register multiple participants?", "Maximum additional participants", "Pending participant expiration", "Allow self-service cancellation or transfer?", "Cancellation or transfer time limit".
My 3rd commit, and similar to the other CiviEvent extras settings, I added a new setting to allow putting a default value for "same email address?" setting when hiding it.
Lastly, I added unit test, fixed the failing tests and updated our test workflow file like this pr MAE-416: Update CiviCRM and specify Drupal version uk.co.compucorp.membershipextras#317 .

omarabuhussein

@ahed-compucorp

left :

1- Regarding the failed tests, you can maybe look into this PR : compucorp/uk.co.compucorp.membershipextras#317
you will probably need to apply the same stuff here for the tests to work.

2- The PR title is not that clear, it is usually good idea to make it as descrptive as possible, maybe in this case something like :

Make showing/hiding events "same email address?" setting configurable

3- It is usually a good idea to show the effect of your work on the places it touches, you are already showing the gif for the settings pages which is good, but can you also show how changing the value will affect the event creation page.

4- I think the overview section does not reflect what this PR is about, actually, I think you can remove it in this PR case.

5- In the technical details part, I find it usually more clear to divide different parts into different points to make it more clear and readable as below (I also made some adjustment to make it more clear) :

- My first commit refactored the old way which depends on saving the CSS class of the element in EventsExtras.setting.php. Now it is doing that as well as sets the default values inside the buildform hook completely.

-  My 2nd commit fixes a bug I found which results in not putting the default values for some of the  "CiviEvent Extras"  Settings. (BTW can you add more notes about that)

- Lastly, and similar to the other  Civievent extras settings, I added a new setting to allow putting a default value for "same email address?" setting when hiding it.

6- lastly, it is better to write descriptive commit messages, so I would change the commit messages you have to

MAE-398: Refactor hideField to MAE-398: Change the way in which event fields are hidden

MAE-398: Fix missing defaults to ```MAE-398: Fix missing default values for hidden event settings"

MAE-398: Add allow_same_participant_emails_default setting to ```MAE-398 : Make 'same email address' default value configurable````

omarabuhussein · 2020-12-15T18:14:02Z

CRM/EventsExtras/Hook/BuildForm/EventFee.php

+    $settings = [$showPayLater];
+    $settingValues = SettingsManager::getSettingsValue($settings);
+    if ($settingValues[$showPayLater] == 0) {
+      $this->hideField('is_pay_later');


I think it would be better if you change hideField method to become hideFields and to make it take an array of ids instead of just one id, if you do that then you could replace these lines to something like this :

$fieldIdsToHide = [ 'is_pay_later', 'pay_later_text', ..etc ]; $this->hideFields($fieldIdsToHide);

omarabuhussein · 2020-12-15T18:26:42Z

CRM/EventsExtras/Hook/BuildForm/BaseEvent.php

-    }
-    $form->assign('hiddenCssClasses', $hiddenFields);
-  }
+    $class = $this->eventTabAndClassMap[$this->eventTab] . '-form-block-' . $id;


can you remove the extra space before this line

omarabuhussein · 2020-12-15T18:50:57Z

tests/phpunit/CRM/EventsExtras/Hook/BuildForm/EventRegistrationTest.php

+  public function setUp() {
+    $formController = new CRM_Core_Controller();
+
+    $insertEvent = "


when dealing with CiviCRM core entities, we usually use CiviCRM API and not direct SQL queries, since it is generally safer to use them as well as some other advantages. You can also abstract that into a fabricator and put it here : https://github.com/compucorp/uk.co.compucorp.eventsextras/tree/80cb477c5da2404d419e06c7077d5733fb4edcce/CRM/EventsExtras/Test/Fabricator

you can maybe see more examples on that in membershipextras extension : https://github.com/compucorp/uk.co.compucorp.membershipextras/tree/master/CRM/MembershipExtras/Test/Fabricator

so the end result for this class would be maybe something like this :

<?php use CRM_EventsExtras_Test_Fabricator_Event as EventFabricator; use CRM_EventsExtras_Test_Fabricator_Setting as SettingFabricator; use CRM_EventsExtras_SettingsManager as SettingsManager; require_once __DIR__ . '/../../../../BaseHeadlessTest.php'; /** * Class CRM_EventsExtras_Hook_BuildForm_EventRegistrationTest * * @group headless */ class CRM_EventsExtras_Hook_BuildForm_EventRegistrationTest extends BaseHeadlessTest { /** * @var CRM_Event_Form_ManageEvent_EventRegistration */ private $eventRegistrationForm; public function setUp() { $event = EventFabricator::fabricate(); $formController = new CRM_Core_Controller(); $this->eventRegistrationForm = new CRM_Event_Form_ManageEvent_Registration(); $this->eventRegistrationForm->controller = $formController; $this->eventRegistrationForm->set('id', $event['id']); $this->eventRegistrationForm->buildForm(); } public function testSetDefaultValues() { SettingFabricator::fabricate([ SettingsManager::SETTING_FIELDS['REGISTER_MULTIPLE_PARTICIPANTS'] => 0, SettingsManager::SETTING_FIELDS['REGISTER_MULTIPLE_PARTICIPANTS_DEFAULT'] => 1, SettingsManager::SETTING_FIELDS['REGISTER_MULTIPLE_PARTICIPANTS_DEFAULT_MAXIMUM_PARTICIPANT'] => 5, SettingsManager::SETTING_FIELDS['REGISTER_MULTIPLE_PARTICIPANTS_ALLOW_SAME_PARTICIPANT_EMAILS_DEFAULT'] => 1, ]); $eventRegistrationFormHook = new CRM_EventsExtras_Hook_BuildForm_EventRegistration(); $eventRegistrationFormHook->handle('CRM_Event_Form_ManageEvent_Registration', $this->eventRegistrationForm); $this->assertEquals(1, $this->eventRegistrationForm->_defaultValues['is_multiple_registrations']); $this->assertEquals(5, $this->eventRegistrationForm->_defaultValues['max_additional_participants']); $this->assertEquals(1, $this->eventRegistrationForm->_defaultValues['allow_same_participant_emails']); } }

Those fields have missing default value: currency, is_multiple_registrations, max_additional_participants, expiration_time, allow_selfcancelxfer, selfcancelxfer_time is_pay_later, pay_later_text, pay_later_receipt, is_billing_required

ahed-compucorp · 2020-12-16T14:54:02Z

Here some notes about about some problems I solved with the template civicrm/templates/CRM/Event/Form/ManageEvent/Registration.tpl :

Has some bugs related to not using the same variable name exactly as part of the CSS class :
- crm-event-manage-registration-form-block-selfcancelxfer instead of crm-event-manage-registration-form-block-allow_selfcancelxfer
- crm-event-manage-registration-form-block-maximum_additional_participants instead of crm-event-manage-registration-form-block-max_additional_participants
The is_billing_required's parent tr doesn't have any CSS class.
Both "Pay Later Label" and "Pay Later Instructions" have their description outside their parent tr.

omarabuhussein · 2020-12-17T10:03:27Z

CRM/EventsExtras/Hook/BuildForm/EventFee.php

+      // @note is_billing_required's parent tr in civicrm/templates/CRM/Event/Form/ManageEvent/Registration.tpl
+      // doesn't have any CSS class (CiviCRM bug) as a workaround we could use another selector
+      // $fieldIdsToHide[] = 'is_billing_required';
+      $this->hideElements('#payLaterOptions tr:nth-child(5)');


as we discussed in the call, this won't be needed since you are hiding the whole parent below, also it seems you don't even need these lines above :

$fieldIdsToHide[] = 'is_pay_later'; $fieldIdsToHide[] = 'pay_later_text'; $fieldIdsToHide[] = 'pay_later_receipt';

since again, you are hiding the whole parent

I only needed the first one because it is not a child of the element tr#payLaterOptions

$fieldIdsToHide[] = 'is_pay_later';

omarabuhussein · 2020-12-17T10:13:16Z

CRM/EventsExtras/Test/Fabricator/Event.php

+   */
+  protected static $defaultParams = [
+    'title'  => 'Event Sample' ,
+    'event_type_id'  => 3 ,


there is no guarantee that an event type with id = 3 will be there, so you will need to create an event type first and use it instead.

I would change the fabricate method to do that :

public static function fabricate(array $params = []) { $eventType = self::createEvenType(); $eventTypeId = $eventType['id']; .. rest of the code } private static function createEvenType() { // use API to create event type here }

omarabuhussein · 2020-12-17T10:15:16Z

CRM/EventsExtras/Test/Fabricator/Event.php

+   * @throws \CiviCRM_API3_Exception
+   */
+  public static function fabricate(array $params = []) {
+    // start_date should always be the future


that's not true, the end date should always be in the future but not the start date, but the end date is not required anyway, so I will change this to :

$start_date = new DateTime();

omarabuhussein · 2020-12-17T10:16:30Z

tests/phpunit/CRM/EventsExtras/Hook/BuildForm/EventRegistrationTest.php

+  }
+
+  public function testSetDefault() {
+


please remove this space

omarabuhussein · 2020-12-17T10:17:43Z

CRM/EventsExtras/Hook/BuildForm/BaseEvent.php

+   * @param string $selector
+   *
+   */
+  protected function hideElements($selector) {


I would rename this to hideElementBySelector

…is enough

omarabuhussein · 2020-12-18T10:51:21Z

CRM/EventsExtras/Test/Fabricator/Event.php

    $params = array_merge($defaultParams, $params);

    return parent::fabricate($params);
  }

+  private static function createEvenType() {


typo : createEvenType should be createEventType (missing T)

omarabuhussein · 2020-12-18T10:51:46Z

CRM/EventsExtras/Test/Fabricator/Event.php

+      'label' => 'Conference',
+      'weight' => 1,
+      'is_active' => 1,
+      'is_reserved' => 1,


no neeed for this to be reserved

ahed-compucorp changed the title ~~MAE-398: Event extra hide "same email address" setting~~ MAE-398: Hide "same email address" setting Dec 15, 2020

omarabuhussein requested changes Dec 15, 2020

View reviewed changes

ahed-compucorp changed the title ~~MAE-398: Hide "same email address" setting~~ MAE-398: Make showing/hiding events "same email address?" setting configurable Dec 16, 2020

ahed-compucorp added 5 commits December 16, 2020 13:27

MAE-398: Change the way in which event fields are hidden

106ec66

MAE-398: Make 'same email address' default value configurable

4e96f2c

MAE-398: Fix and add unit tests

81014ac

MAE-398: Update CiviCRM and specify Drupal version

774675b

ahed-compucorp force-pushed the MAE-398-events-extras-hide-setting branch from 80cb477 to 774675b Compare December 16, 2020 12:56

ahed-compucorp added 2 commits December 16, 2020 15:53

MAE-398: Fix start date to be in the future for EventFabricator

47c2904

MAE-398: Fix linter errors

552f423

omarabuhussein requested changes Dec 17, 2020

View reviewed changes

ahed-compucorp added 3 commits December 17, 2020 12:55

MAE-398: Remove unnecessary code because hiding the the whole parent …

258e38a

…is enough

MAE-398: Rename hideElements method and update places where it is used

164dfcc

MAE-398: Create a new EventType in test instead of hard coding it

5427d91

omarabuhussein requested changes Dec 18, 2020

View reviewed changes

MAE-398: Fix a typo && remove unnecessary fields

e21eae3

omarabuhussein approved these changes Dec 18, 2020

View reviewed changes

ahed-compucorp merged commit 4d0ae41 into master Dec 22, 2020

ahed-compucorp deleted the MAE-398-events-extras-hide-setting branch December 22, 2020 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAE-398: Make showing/hiding events "same email address?" setting configurable #21

MAE-398: Make showing/hiding events "same email address?" setting configurable #21

ahed-compucorp commented Dec 15, 2020 •

edited

Loading

omarabuhussein left a comment

omarabuhussein Dec 15, 2020

omarabuhussein Dec 15, 2020

omarabuhussein Dec 15, 2020

ahed-compucorp commented Dec 16, 2020 •

edited

Loading

omarabuhussein Dec 17, 2020

ahed-compucorp Dec 18, 2020

omarabuhussein Dec 17, 2020

omarabuhussein Dec 17, 2020

omarabuhussein Dec 17, 2020

omarabuhussein Dec 17, 2020

omarabuhussein Dec 18, 2020

omarabuhussein Dec 18, 2020

MAE-398: Make showing/hiding events "same email address?" setting configurable #21

MAE-398: Make showing/hiding events "same email address?" setting configurable #21

Conversation

ahed-compucorp commented Dec 15, 2020 • edited Loading

Before

After

Technical Details

omarabuhussein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahed-compucorp commented Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahed-compucorp commented Dec 15, 2020 •

edited

Loading

ahed-compucorp commented Dec 16, 2020 •

edited

Loading