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

Fs/error on dob #26814

Closed
wants to merge 5 commits into from
Closed

Fs/error on dob #26814

wants to merge 5 commits into from

Conversation

sertlab
Copy link
Contributor

@sertlab sertlab commented Feb 11, 2020

This PR fixes this issue
#26700

How to test

  • Set DOB as a required during customer registration and try to register

Actual Result
Registration fails with message (under the calendar) "Please select a valid date"

Expected Result
Registration should be complete

What changed
preg_replace() modified in order moment js to accept the date format value
as it seems moment(date,'M/d/Y').isValid() returns false
but now moment(date,'M/dd/Y').isValid() returns true

@m2-assistant
Copy link

m2-assistant bot commented Feb 11, 2020

Hi @sertlab. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@mrtuvn
Copy link
Contributor

mrtuvn commented Feb 12, 2020

@sertlab Can you cover your change with unit test ?
The last change of file Timezone in this PR
#26701

@sertlab
Copy link
Contributor Author

sertlab commented Feb 12, 2020

@sertlab Can you cover your change with unit test ?
The last change of file Timezone in this PR
#26701

some unit test coverage added to this .

@sertlab sertlab self-assigned this Feb 13, 2020
@ghost ghost unassigned sertlab Feb 13, 2020
@ghost
Copy link

ghost commented Feb 13, 2020

@sertlab unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

@mrtuvn mrtuvn requested a review from jameshalsall February 23, 2020 02:07
@mrtuvn
Copy link
Contributor

mrtuvn commented Feb 23, 2020

@sertlab Can you run test case for class
lib/internal/Magento/Framework/Stdlib/DateTime/Timezone.php
Add some case to scopeDateDataProvider in this class
https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/Stdlib/Test/Unit/DateTime/TimezoneTest.php#L311

@bubasuma
Copy link
Contributor

Hi @sertlab
Although replacing M/d/Y by M/dd/Y works for moment().isValid(). This is not really the moment JS equivalent of M/d/Y. See https://momentjs.com/docs/#/displaying/
I did some basic test with momentJS date formatting. See screenshot below
Screen Shot 2020-03-12 at 9 14 18 AM

I believe the momentJS equivalent of M/d/Y should be M/D/Y. See screenshot below
Screen Shot 2020-03-12 at 9 25 07 AM

@bubasuma
Copy link
Contributor

Hi @sertlab
Thank you for your contribution and collaboration!
Internal Magento team is working on this issue right now.

@bubasuma
Copy link
Contributor

bubasuma commented Apr 2, 2020

Hi @sertlab
Thank you for your contribution and collaboration!
The issue was fixed by Magento team.
The fix will be available with the upcoming 2.4.0 release.

@bubasuma bubasuma closed this Apr 2, 2020
@m2-assistant
Copy link

m2-assistant bot commented Apr 2, 2020

Hi @sertlab, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

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.

4 participants