-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
CRM-20922 fix support for passing custom date values via url #11868
Conversation
So the difference here is that we're not attempting to transform the date format; rather we're just validating that it's of in ISO format, and then if not we're just ignoring it? Yeah, I like that. I haven't actually tried it, but code and test look good to me. Is that helpful? |
@twomice I guess I need you (or someone) to test it (& preferably fill in a review template on it) before I can merge it |
This should be tested on a system with non-ISO date formats. |
@JoeMurray it works on non ISO date formats but the url date needs to be in ISO format. I think that is reasonable & since it has not been working for a while it's not a 'change' per se |
@twomice just noting that this probably won't get merged unless you test it & I will close it if it hangs around for a month or longer (I'm finding a lot of my time is getting sucked into getting work merged that I actually have no interest in so I want to get this & 3 others resolved even if it means closing them unmerged (the others are #11720 & #11887 & #11884 ) |
Yeah, I get it; yuck wasting time. I think a month is gracious. Thanks. |
@twomice tick tick |
Jenkins, retest this please. |
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.
(CiviCRM Review Template MC-1.1)
- Explain (
r-explain
)- PASS : The goal/problem/solution have been adequately explained with a link (JIRA, Github, Gitlab, StackExchange).
- Test results (
r-test
)- Code quality (
r-code
)- PASS: The functionality, purpose, and style of the code seems clear+sensible.
- COMMENTS: Nice to see replacement of 13 LoC with one call to an existing method.
- Code quality (
- Documentation (
r-doc
)- PASS: There are relevant updates for the documentation, or the changes do not require documentation.
- COMMENTS: Documentation would be nice, but where? Anyway, I don't want to block this with nice-to-have documentation; any lack of documentation doesn't cause breakage in this case.
- Maintainability (
r-maint
)- PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
- Run it (
r-run
)- PASS:
- On dmaster.demo.civicrm.org:
- Added "Marriage date" custom field (id=3) to "Supporter Information" profile, and added "Supporter Information" profile to Contribution Page id=1
- Visited http://[site-url]/civicrm/contribute/transact?reset=1&id=1&custom_3=2010-09-04
- Observed that indeed the &custom_3 query parameter is ignored, and the "marriage date" field has no default value.
- Re-ran jenkins tests and visited testing site.
- On jenkins testing site:
- Added "Marriage date" custom field (id=3) to "Supporter Information" profile, and added "Supporter Information" profile to Contribution Page id=1
- Visited http://[site-url]/civicrm/contribute/transact?reset=1&id=1&custom_3=2010-09-04
- Observed that the &custom_3 query parameter is honored, causing the "marriage date" field to have a default value of "09/04/2010"
- Edited the configuration for the "marriage date" custom field, to use a date format of "dd/mm/YYYY"
- Visited http://[site-url]/civicrm/contribute/transact?reset=1&id=1&custom_3=2010-09-04
- Observed that the &custom_3 query parameter is still honored correctly, causing the "marriage date" field to have a default value of "04/09/2010"
- On dmaster.demo.civicrm.org:
- PASS:
- User impact (
r-user
)- PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
- COMMENTS: Only a little sad that documentation doesn't make people aware of this feature.
- Technical impact (
r-tech
)- PASS: The change preserves compatibility with existing callers/code/downstream.
- COMMENTS: IIRC, older version supported varying date formats in the query parameter value, and this requires ISO format. But since this feature has been non-existent for several versions, I'm chosing to see it as a new feature, not to be compared with some similar past feature.
Thanks @twomice - merging |
Thanks @eileenmcnaughton ! |
Overview
Re-instate support for passing custom date data via a url
Before
Passing a pre-fill value for a custom date via a url does not 'work'
eg.
http://dmaster.local/civicrm/contribute/transact?reset=1&id=1&custom_3=2010-09-03
After
Custom Date in url does work - from the url this has populated
(I tested with an invalid date & it didn't prefill)
Technical Details
@twomice - this is a reviewer's cut of your PR #10702 - the key difference is I have re-instated support but only for the iso format in the url. This is simpler to maintain & after the big break in this working it feels Ok to change the format to something easier to support
Comments
I took your test & slightly tweaked it to use the different format. Please check you are OK with my tweaked version & I will merge based on your confirmation.