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

Use addDatePickerRange instead of deprecated/removed addDateRange() for forms #181

Merged
merged 1 commit into from Nov 20, 2023

Conversation

jensschuppe
Copy link
Collaborator

@jensschuppe jensschuppe commented Oct 10, 2023

Fixes #138.

This replaces the custom-built date range selector fields in task forms with addDatePickerRange() as suggested by the deprecation notice, making custom JS code for translating custom relative date options into actual date values unnecessary.

This effectively adds all available date range options and uses CRM_Utils_Date::getFromTo() for converting relative dates (previous fiscal year etc.) into from and to values to work with the extension's current query building.

Note that the date picker element can apparently not be made required, as all sub-fields would be required effectively (_from, _to, _relative). Maybe there should be additional validation for that …?

I'd appreciate some functional review (maybe by @jojowork?) and a code-one (@bjendres?).

systopia-reference: 23334

@jensschuppe jensschuppe added enhancement status:needs review Code needs review and testing. labels Oct 10, 2023
@jensschuppe jensschuppe added this to the 2.3 milestone Oct 10, 2023
'donrec_contribution_horizon',
E::ts('Time period'),
FALSE,
FALSE,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As pointed out in the PR summary, setting this to TRUE causes all sub-fields be required, which makes selecting relative date options impossible.

Comment on lines +41 to +42
'_to',
'_from',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those are _low and _high by default, thus we have to pass the traditional field names here.

$values['donrec_contribution_horizon_from'],
$values['donrec_contribution_horizon_to']
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This processes relative dates and formats from and to values with YmdHiS, unfortunately, that's why we have to re-format again below.

Comment on lines +40 to +41
$date_from = CRM_Utils_DonrecHelper::convertDate($raw_from_ts, -1, 'YmdHis');
$date_to = CRM_Utils_DonrecHelper::convertDate($raw_to_ts, 1, 'YmdHis');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not change the parameter's default value in the signature, although it does not seem to be called from anywhere else than here.

@jensschuppe jensschuppe changed the title Use addDatePickerRange instead of deprecated(removed addDateRange() for forms Use addDatePickerRange instead of deprecated/removed addDateRange() for forms Oct 10, 2023
@Luensche
Copy link

I have just changed the files in my installation, so it is only quick and dirty, but I still get an exception:

Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in /var/www/html/wp-content/uploads/civicrm/ext/de.systopia.donrec-2.2.0/CRM/Donrec/Form/Task/ContributeTask.php:123 Stack trace: #0 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/Form.php(624): CRM_Donrec_Form_Task_ContributeTask->postProcess() #1 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/StateMachine.php(144): CRM_Core_Form->mainProcess() #2 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/QuickForm/Action/Next.php(43): CRM_Core_StateMachine->perform(Object(CRM_Donrec_Form_Task_ContributeTask), 'next', 'Next') #3 /var/www/html/wp-content/plugins/civicrm/civicrm/packages/HTML/QuickForm/Controller.php(203): CRM_Core_QuickForm_Action_Next->perform(Object(CRM_Donrec_Form_Task_ContributeTask), 'next') #4 /var/www/html/wp-content/plugins/civicrm/civicrm/packages/HTML/QuickForm/Page.php(103): HTML_QuickForm_Controller->handle(Object(CRM_Donrec_Form_Task_ContributeTask), 'next') #5 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/Controller.php(355): HTML_QuickForm_Page->handle('next') #6 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/Invoke.php(319): CRM_Core_Controller->run(Array, NULL) #7 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/Invoke.php(69): CRM_Core_Invoke::runItem(Array) #8 /var/www/html/wp-content/plugins/civicrm/civicrm/CRM/Core/Invoke.php(36): CRM_Core_Invoke::_invoke(Array) #9 /var/www/html/wp-content/plugins/civicrm/civicrm.php(1199): CRM_Core_Invoke::invoke(Array) #10 /var/www/html/wp-includes/class-wp-hook.php(310): CiviCRM_For_WordPress->invoke('') #11 /var/www/html/wp-includes/class-wp-hook.php(334): WP_Hook->apply_filters('', Array) #12 /var/www/html/wp-includes/plugin.php(517): WP_Hook->do_action(Array) #13 /var/www/html/wp-admin/admin.php(259): do_action('toplevel_page_C...') #14 {main} thrown in /var/www/html/wp-content/uploads/civicrm/ext/de.systopia.donrec-2.2.0/CRM/Donrec/Form/Task/ContributeTask.php on line 123

Do I have to run an upgrader or delete sth?

@jensschuppe
Copy link
Collaborator Author

Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in /var/www/html/wp-content/uploads/civicrm/ext/de.systopia.donrec-2.2.0/CRM/Donrec/Form/Task/ContributeTask.php:123 is a different error that has been fixed and merged with #180. I rebased onto the current master - please try once again!

Please note that you should not include versions in your extension directory names (ext/de.systopia.donrec-2.2.0) - we include installable archives with clean directory names in all our releases (ext/de.systopia.donrec). This saves you from accidentally placing the same extension twice in your ext directory.

@Luensche
Copy link

I've tested the changes again after the rebase and it looks good.

@jensschuppe jensschuppe added status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. and removed status:needs review Code needs review and testing. labels Nov 20, 2023
@jensschuppe jensschuppe modified the milestones: 2.3, 2.2 Nov 20, 2023
@jensschuppe jensschuppe merged commit 197af68 into master Nov 20, 2023
jensschuppe added a commit that referenced this pull request Nov 20, 2023
@jensschuppe jensschuppe removed the status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. label Nov 20, 2023
jensschuppe added a commit that referenced this pull request Nov 20, 2023
@jensschuppe jensschuppe added the status:fixed The issue has been resolved (usually by committing/merging code). label Nov 20, 2023
@jensschuppe
Copy link
Collaborator Author

Released with version 2.2.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement status:fixed The issue has been resolved (usually by committing/merging code).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[meta] Remove/replace deprecated CiviCRM Core code
2 participants