-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
dev/core#1403 - CRM/Contribute - Change "Date Received" to "Contribution Date" #26145
dev/core#1403 - CRM/Contribute - Change "Date Received" to "Contribution Date" #26145
Conversation
No issue was found matching the number given in the pull request title. Please check the issue number. |
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
(Standard links)
|
I have reviewed. It looks like the consensus in the linked issue was to change the label to "Contribution date" and this changes to "Date", but I think it is clear that it is a contribution date in context and "Contribution date" would be redundant, so that seems better to me. There are also a lot more places where Received Date or just Received is used. I think it would make sense to update all of those at the same time in this PR. Wouldn't it make sense to change the descriptions at the same time? Since we're saying this is not, in some cases, actually the receive date, it doesn't really make sense to have a description that says something about the date the contribution was received. I think different orgs are going to use date differently and we shouldn't be trying to describe specific use cases. I'm in favour of removing the description entirely, both the schema comment and where it appears on forms. |
I agree @larssandergreen about the approved label change. I think |
Agree! |
98bba95
to
a87ebe9
Compare
jenkins test this please |
jenkins retest this please |
Just commenting based on the discussion this would be mergeable, but I haven't merged yet because the tests haven't run and I can't seem to get them to run (note it only says 2 checks have run - those are the style check and test site build, but not the actual unit tests). I think maybe an admin needs to add to whitelist to get them to run? |
There are also a few in the tests here and here that will need to be updated. There are also two instances of "Received" in I think that's it. |
Jenkins add to whitelist |
8dcdcd1
to
d9ad017
Compare
Thanks for the review @larssandergreen, The necessary "Received" or "Received Date" fields have all been changed to "Contribution Date". Except for those in https://github.com/civicrm/civicrm-core/blob/master/sql/civicrm_generated.mysql, running the GenCode script doesn't update them. |
To update civicrm_generated.mysql, you need to run But when you update civicrm_data.tpl, you also need to update this data on existing installations (since this file is only used on install), which you do by adding SQL in |
Note when you run regen.sh it will then fail unrelated tests because a recent membership wording change was merged that needs some tests updated first. It's a bit messy right now. Note you can also run regen online in your fork: see https://github.com/civicrm/civicrm-core/blob/master/.github/workflows/regen.yml |
45c6f4f
to
75b45d5
Compare
@@ -12,3 +12,6 @@ INSERT IGNORE INTO civicrm_navigation | |||
VALUES | |||
( {$domainID}, 'https://docs.civicrm.org/user/?src=iam', '{ts escape="sql" skip="true"}User Guide{/ts}', 'User Guide', NULL, 'AND', @adminHelplastID, '1', NULL, 1 ), | |||
( {$domainID}, 'https://civicrm.org/help?src=iam', '{ts escape="sql" skip="true"}Get Help{/ts}', 'Get Help', NULL, 'AND', @adminHelplastID, '1', NULL, 2 ); | |||
|
|||
{* https://github.com/civicrm/civicrm-core/pull/26145 *} | |||
UPDATE civicrm_uf_field SET label = '{ts escape="sql"}Contribution Date{/ts}' WHERE field_name = 'receive_date'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is failing because this would need to be done in php and accounting for multilingual where the field is called label_enUS, etc, like in FiveThirtyOne.php. But I think it would also be ok to leave it out so we can get this merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE civicrm_uf_field SET label = '{ts escape="sql"}Contribution Date{/ts}' WHERE field_name = 'receive_date'; | |
{if $multilingual} | |
{foreach from=$locales item=locale} | |
UPDATE IGNORE civicrm_uf_field SET label_{$locale} = '{ts escape="sql"}Contribution Date{/ts}' WHERE field_name = 'receive_date'; | |
{/foreach} | |
{else} | |
UPDATE IGNORE civicrm_uf_field SET label = '{ts escape="sql"}Contribution Date{/ts}' WHERE field_name = 'receive_date'; | |
{/if} |
Sorry, I should have thought to mention that. This should do the job. It still works in SQL, @demeritcowboy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it does work, I tested it. But there's no specific reason we can't still use SQL here, is what I meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larssandergreen, thanks for the suggestion it looks good, is it okay to commit?
or do we want to leave it as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't be left as is, and now there is also a merge conflict. Either sql or php but also it should only change it if the current label is Date Received, otherwise it will overwrite someone's preferred wording if they already changed it in the profile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @olayiwola-compucorp it's still failing tests since this is the same code as before. Can I suggest leaving this out and then we can merge the rest? You can always put up another PR after.
Would this also change the name of the token? from {contribution.receive_date} to {contribution.date} |
@magnolia61 This change wouldn't affect the name of the token, that would require making changes to the schema. This PR only changes the label/title the field name still remains as |
75b45d5
to
10dde3e
Compare
10dde3e
to
395314f
Compare
Yay. @olayiwola-compucorp Can you make a PR to add yourself to contributor-key.yml for release notes credit? |
Thanks @demeritcowboy, done #26225 |
Included in CiviCRM 5.63 PR: civicrm#26145
Included in CiviCRM 5.63 PR: civicrm#26145
Great job @olayiwola-compucorp and a big thank you to everyone on this ticket who has got this over the line. One small change for civi, a giant leap for civi-kind! |
Included in CiviCRM 5.63 PR: civicrm#26145
Overview
Development work for https://lab.civicrm.org/dev/core/-/issues/1403
This PR updates the Contribution
receive_date
title/label fromDate Received
toContribution Date
so it's clear that the date is the contribution date. Also, the receive_date description /commenthas been removed.Before
The contribution
receive_date
has the titleDate Received
orReceived
.After
The contribution
receive_date
title has now been changed toContribution Date
.e.g.
New/Edit Contribution Screen
Contact Contributions
Contribution Searckit
New Membership Screen
In summary, the changes will affect screens such as the Contribution Import/Export screen which previously used "Date Received" or "Received" as a label for the Contribution "receive_date" field.