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

CRM-21050: Use datepicker for all date fields used in contribution backoffice form #10841

Merged
merged 2 commits into from
Jan 6, 2018

Conversation

pradpnayak
Copy link
Contributor

@pradpnayak pradpnayak commented Aug 9, 2017

Overview

Use datepicker for datefields used in backoffice contribtution, these are:

  1. Recieve Date
  2. Thank you Date
  3. Receipt Date
  4. Cancel/Refund Date

Before

Date fields were using jcalender.

After

Replaced all the date fields with datepicker. Here's the screencast to show that default date values and correctly set after submission:
test-multiple-after


NOTE original PR was tackling this in a different way - hence comments don't appear to make sense.

Copy link
Member

@monishdeb monishdeb left a comment

Choose a reason for hiding this comment

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

Tested, working fine.

@eileenmcnaughton
Copy link
Contributor

This makes me sad. We really want to switch to datepicker rather than deal with this nasty handling.

However, I'm ok for this to be merged after one clarification. It seems to me it is not receive_date (per the title of the PR) but rather trxn_date that is not being set correctly. Is that correct? If so can we correct that in the PR / JIRA?

@mattwire
Copy link
Contributor

mattwire commented Aug 12, 2017

@eileenmcnaughton

This makes me sad. We really want to switch to datepicker rather than deal with this nasty handling.

Agreed, but I've abandoned datepicker so many times as I cannot, for want of trying, get it to set a default date passed in from CiviCRM. With the old method this is easy!

@eileenmcnaughton
Copy link
Contributor

is this actually adding trxn_date to the form? If so it seems a bigger change than I realised

@eileenmcnaughton
Copy link
Contributor

@mattwire - this patch works on that form to convert thank you date & loads the default ok - does it offer you any clues as to the problem you were having?

eileenmcnaughton@d0177d8

Note that the Contribution.xml already has metadata to describe the date field so I didn't need to add it

@JoeMurray
Copy link
Contributor

@eileenmcnaughton there are three date fields displayed on the form, this ensures that all three have their correct time displayed rather than 12:00am for the part of the form we're talking about is the payment section. The before and after images show the payment trxn_date now get their time displayed unlike before. receive_date processing should be unchanged.

@eileenmcnaughton
Copy link
Contributor

Right - but I think if we fix any date fields we should change them to use the preferred datepicker at that point. We shouldn't fix the deprecated date field version - the patch above was the example of how to do that for one field

@monishdeb monishdeb force-pushed the CRM-21050 branch 2 times, most recently from 2aa9de2 to c08d817 Compare January 1, 2018 13:43
@colemanw
Copy link
Member

colemanw commented Jan 1, 2018

@mattwire the confusing thing about switching to the new datpicker is that it uses a different format than the old method, which might mess up the defaults if you're dealing with legacy code. The old fields would use a localized format across 2 fields (one for date, one for time) which got complicated and messy. The new datepicker widget contains a standard ISO date/time value, which should be much easier to work with once you clear away the legacy stuff.

@monishdeb
Copy link
Member

Jenkins test this please

@monishdeb monishdeb changed the title CRM-21050, fixed code to set receive date with correct timestamp CRM-21050: Use datepicker for all date fields used in contribution backoffice form Jan 1, 2018
@monishdeb
Copy link
Member

monishdeb commented Jan 1, 2018

@eileenmcnaughton @colemanw I have changed the JIRA title and description, because as per change it's an improvement to use datepicker for all date fields used in backoffice contribution. Also, I have shared a screencast above to show that the date field values are correctly submitted and set as default on the edit form.

@monishdeb monishdeb force-pushed the CRM-21050 branch 2 times, most recently from 52daeb8 to 834e55b Compare January 1, 2018 18:58
@@ -757,23 +757,18 @@ public function testbyActivityDateAndStatus() {
$lastWeekActivities = array(1, 2, 3);
$todayActivities = array(4, 5, 6, 7);
$lastTwoMonthsActivities = array(8, 9, 10, 11);
$lastYearActivties = array(12, 13, 14, 15, 16);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be in a separate PR - even if we have to ignore the test fail for now

@@ -1203,8 +1203,6 @@ public function createContributionWithTax() {
$form->testSubmit(array(
'total_amount' => 100,
'financial_type_id' => $financialType['id'],
'receive_date' => '04/21/2015',
Copy link
Contributor

Choose a reason for hiding this comment

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

can we update the format on these dates rather than remove?

Copy link
Member

Choose a reason for hiding this comment

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

@eileenmcnaughton 2 reasons why I removed it:

  1. Provided the default value of current date time in the desired format here
  2. In these unit-tests receive date has no importance, however I have retained in some UT like here where it's needed.

Or do you still want to update their formats?

Copy link
Contributor

Choose a reason for hiding this comment

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

that seems fine - sounds like we are not losing any test data

@eileenmcnaughton
Copy link
Contributor

Looks solid from a code POV - I haven't tested

----------------------------------------
* CRM-21050: credit card invoice: receive time set to 12:00
  https://issues.civicrm.org/jira/browse/CRM-21050
@monishdeb
Copy link
Member

@eileenmcnaughton I have moved the additional UT failure fix in separate PR #11479

@monishdeb
Copy link
Member

Jenkin test this please

@eileenmcnaughton
Copy link
Contributor

test failure is unrelated

@colemanw
Copy link
Member

colemanw commented Jan 4, 2018

@civicrm-builder retest this please

@colemanw
Copy link
Member

colemanw commented Jan 4, 2018

I'm satisfied with the amount of review this has seen. Adding "merge on pass" label.

@monishdeb
Copy link
Member

monishdeb commented Jan 5, 2018

Test build failure is not related and fixed here #11479

@eileenmcnaughton
Copy link
Contributor

Merging in accordance with merge on pass as fail is unrelated

@eileenmcnaughton eileenmcnaughton merged commit ba5e20d into civicrm:master Jan 6, 2018
@monishdeb monishdeb deleted the CRM-21050 branch January 6, 2018 15:59
@monishdeb
Copy link
Member

Thanks @eileenmcnaughton and @colemanw for your review and getting it merged :)

@jitendrapurohit
Copy link
Contributor

Fatal error is displayed when a user tries to delete a contribution after this change. Raised a fix at #11500

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21050: Use datepicker for all date fields used in contribution backoffice form
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants