Adds 'reset status' functionality #501
Conversation
|
This needs systopia/civicrm-extension-template#4 be implemented here for the PHPUnit action not to fail running, and it'll help to get the PHP_CodeSniffer and PHPStan actions come back green as well for merging this sooner. |
| } | ||
|
|
||
| // allow trx status reset | ||
| Civi::settings()->set('allow_trx_reset', !empty($values['allow_trx_reset'])); |
There was a problem hiding this comment.
Instead of empty() you could use (bool) ($values['allow_trx_reset'] ?? FALSE).
There was a problem hiding this comment.
At least for me
!empty($values['allow_trx_reset'])is more readable than
(bool) ($values['allow_trx_reset'] ?? FALSE)| } | ||
| } | ||
|
|
||
| $allow_trx_reset = CRM_Core_BAO_Setting::getItem('CiviBanking', 'allow_trx_reset'); |
There was a problem hiding this comment.
I think Civi::settings()->get('allow_trx_reset') would be preferable.
| * The metadata is used for setting defaults, documentation & validation | ||
| * @param array $params array or parameters determined by getfields | ||
| */ | ||
| function _civicrm_api3_banking_transaction_reset_spec(&$params) { |
There was a problem hiding this comment.
I am unsure about type hints here.
Core has a lot of _civicrm_api3_XYZ_spec() function all without type hints.
| * @param array $params array or parameters determined by getfields | ||
| */ | ||
| function _civicrm_api3_banking_transaction_reset_spec(&$params) { | ||
| $params['trx_id'] = array( |
| * Resets a transaction's status into the status NEW | ||
| * Will also remove any suggestions stored into this transaction | ||
| * | ||
| * @param mixed $params |
There was a problem hiding this comment.
I think $params should be an array, not mixed.
| {assign var="resetLink" value=""} | ||
|
|
||
| {if $btxstatus.label eq 'Ignored'} | ||
| {if $allow_trx_reset} |
There was a problem hiding this comment.
Maybe it makes sense to combine it with the if before.
| return; | ||
| } | ||
|
|
||
| var reload_regex = new RegExp("(execute=)[0-9]+", 'ig'); |
There was a problem hiding this comment.
var should be avoided in favor if const and let.
There was a problem hiding this comment.
I agree, we should use const (I think).
| return; | ||
| } | ||
|
|
||
| var reload_regex = new RegExp("(execute=)[0-9]+", 'ig'); |
There was a problem hiding this comment.
camelCase for reload_regex?
There was a problem hiding this comment.
Actually I am unsure about the proper use of camelCase in CiviCRM.
Coming from Drupal I was told "stick to Drupal's style and you're fine".
https://docs.civicrm.org/dev/en/latest/standards/#civicrm-vs-drupal says:
In general, CiviCRM follows the Drupal Coding Standards
I did not found anything CiviCRM specific about naming variables.
The linked https://www.drupal.org/docs/develop/standards is labeled "[Obsolete]" and links to https://project.pages.drupalcode.org/coding_standards/
Both of those say:
Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: $snake_case). Be consistent; do not mix camelCase and snake_case variable naming inside a file.
Since templates/CRM/Banking/Page/Review.tpl uses snake_case for variables, so I think we should stick to it.
|
|
||
| // disable ALL buttons | ||
| cj(".button").addClass('disabled'); | ||
| cj(".button").attr("onclick",""); |
There was a problem hiding this comment.
There should be a space after ,.
| return; | ||
| } | ||
|
|
||
| var reload_regex = new RegExp("(execute=)[0-9]+", 'ig'); |
There was a problem hiding this comment.
The use of double and single quotes isn't consistent. (Here and in the rest of the code.)
I'm not sure what you mean with the second part of the sentence. Anyhow the phpcs and phpstan issues are independent of phpunit. |
This was just to say that we want everything green before merging. |
|
@jensschuppe do we still want this to be part of |
|
Probably not, I removed the milestone (we usually only add milestones when merging or if we really want things to be in the next version or for backports - not sure why I added it here). |
Fixes #383.
Replaces #392 (rebased onto current
master) by @VangelisP, description at time of creation: