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

Fix misformatted start_date default on new contribution page #12504

Merged
merged 1 commit into from
Jul 18, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fix unreleased regression from #11881

see https://lab.civicrm.org/dev/core/issues/263

Before

Start date field does not populate for new contribution page. Bug on save if not set

After

Start date field does populate for new contribution page. Save works

Technical Details

This was overlooked when switching the field to datepicker (preferred) in #11881 . Huge thanks to @MegaphoneJon for finding while in rc - seems that because we were focussed on the handling of various start dates we never tried not setting one.

Comments

@yashodha @MegaphoneJon can one of you review this?

@civibot
Copy link

civibot bot commented Jul 18, 2018

(Standard links)

@MegaphoneJon
Copy link
Contributor

I'll review it - I haven't reviewed many PRs lately.

@eileenmcnaughton
Copy link
Contributor Author

Thanks @MegaphoneJon - happy to help you with upping your pr-review output :-)

Copy link
Contributor

@MegaphoneJon MegaphoneJon left a comment

Choose a reason for hiding this comment

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

@eileenmcnaughton Two issues, one blocking, one not.

First - this avoids the bug, but it will set ANY date to the current date, and not allow a change. You could move the lines up to the beginning of the function, or wrap them in a conditional.

Of course, that still means that contribution pages can no longer have no start date. I can't think of a reason why it would matter if a blank start date is always set to "now" - but it is inconsistent with other fields* thus a (non-blocking) hit to the UX.

  • Except submitting a CiviMail, but that has a good reason.

@eileenmcnaughton
Copy link
Contributor Author

@MegaphoneJon try the updated version - it should only be set for new not for existing - that is consistent with previous behaviour from what I can see

@MegaphoneJon
Copy link
Contributor

@eileenmcnaughton This restores the original bug. I did a quick debug session - when you create a new contribution page, $defaults['start_date'] seems to default to a m/d/Y date format, even though none is displayed in the datepicker - probably because that's the wrong format for the datepicker.

selection_534

@eileenmcnaughton
Copy link
Contributor Author

ug - that doesn't look like my latest cut - it should be

if (!$this-_id) {
   $defaults['start_date'] = date('Y-m-d H:i:s');
   unset($defaults['start_time']);

}

will fix

@eileenmcnaughton
Copy link
Contributor Author

Well - that assumes it is not mis-formttatting any results loaded from $this->_values

Copy link
Contributor

@MegaphoneJon MegaphoneJon left a comment

Choose a reason for hiding this comment

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

I tested this with a number of permutations and was unable to get unexpected behavior/crashes. OK to merge.

@eileenmcnaughton
Copy link
Contributor Author

thanks @MegaphoneJon

@eileenmcnaughton eileenmcnaughton merged commit 6a4b27d into civicrm:5.4 Jul 18, 2018
@eileenmcnaughton eileenmcnaughton deleted the yashi branch July 18, 2018 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants