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

Fixed date display issue after filtration on sales order page. #15367

Closed
wants to merge 3 commits into from

Conversation

vishal-7037
Copy link
Contributor

Description

In date.js file at line no 97, there were two method used to format/shift date value as listens param.
The method onShiftedValueChange is call onValueChange method inside it's body content so there is no need to use both methods.
Because of both methods, the values of date fields are format/shift two times and rises the issue of Invalid Date.

Fixed Issues (if relevant)

  1. Date range filter in admin ui grid displaying "Invalid date" after reload #13428: Date range filter in admin ui grid displaying "Invalid date" after reload

Manual testing scenarios

  1. Try to filter the records of sales orders using date range and then reload page again.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented May 19, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ vishal-7037
❌ vishal-sutariya

@@ -95,7 +95,7 @@ define([
timezoneFormat: 'YYYY-MM-DD HH:mm',

listens: {
'value': 'onValueChange',
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @vishal-7037, thank you for collaboration. We can't remove the handler onValueChange.

The value property has the responsibility to set and get data from the registry.

The registry use as storage to transport data from client to server and conversely, the data from registry send to the server and server puts the data to the registry when page loading.

All UiComponents reads data from registry and put it to local value property.
You can look it inside abstract.js.

The shiftedValue is the only local property that used for UI rendering.

So, in case when we selected some date in datepicker and saved it after page reload the datepicker component will not render it, because the value is exist, but handler for value don't run, and date will not be set to the shiftedValue property.

@vishal-7037
Copy link
Contributor Author

Hello @VladimirZaets ,
I have pushed new changes, please verify it.
Thank you.

@VladimirZaets
Copy link
Contributor

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets, here is your Magento instance.
Admin access: https://pr-15367.engcom.dev.magento.com/admin
Login: admin Password: 123123q
Please make sure you are PR author or assignee to access the instance.

@VladimirZaets
Copy link
Contributor

@magento-engcom-team give me 2.2-develop instance

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets. Thank you for your request. I'm working on Magento 2.2-develop instance for you

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets, here is your Magento instance.
Admin access: https://i-15367-2-2-develop.engcom.dev.magento.com/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

@VladimirZaets
Copy link
Contributor

@vishal-7037
I investigated the issue 13428.
The main problem of this isn't that two handlers are calling. The main problem is in date formats.
The date was passed to onValueChange handler but format convertation failed.
Line 166
shiftedValue = moment(value, dateFormat);

Please, find the solution to this problem.

@vishal-7037
Copy link
Contributor Author

vishal-7037 commented May 21, 2018

Hi @VladimirZaets ,
I have pushed new changes, I have added value parameter to shiftedValue function to set dateFormat based on ternary condition.
After this change shiftedValue = moment(value, dateFormat); working properly.
Please verify it.
Thank you.

@vishal-7037
Copy link
Contributor Author

Hi @VladimirZaets
Have you verified my last change?
Please verify it.

@VladimirZaets
Copy link
Contributor

Hi @vishal-7037, your fix doesn't resolve the problem.
this.shiftedValue(value) will always return true, so the flow with else scenario will never works.

In general, component /form/element/date.js works right, the problem in the data format that it receives, and this case should be resolved by component configuration.

The right solution for this issue is understude why wrong date format was passed and if needed configure the right format.

@okorshenko okorshenko removed this from the May 2018 milestone May 31, 2018
@VladimirZaets
Copy link
Contributor

@vishal-7037, I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for collaboration

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.

7 participants