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 for issue #21477 sets CURRENT_TIMESTAMP on updated_at fields #21486

Merged

Conversation

dverkade
Copy link
Member

Description

Fix for issue #21477 sets CURRENT_TIMESTAMP on updated_at fields in the DB Schema.

Fixed Issues (if relevant)

  1. Magento 2.3 quote_item table has incorrect default value in declarative schema #21477: Magento 2.3 quote_item table has incorrect default value in declarative schema

Manual testing scenarios

Check database after creating a quote and placing an item in the quote that updated at field is filled correctly.

@magento-engcom-team
Copy link
Contributor

Hi @dverkade. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@okorshenko
Copy link
Contributor

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @okorshenko, here is your new Magento instance.
Admin access: https://pr-21486.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@magento-engcom-team
Copy link
Contributor

Hi @miguelbalparda, thank you for the review.
ENGCOM-4454 has been created to process this Pull Request

@soleksii
Copy link

✔️ QA Passed

@@ -107,7 +107,7 @@
comment="Oauth consumer"/>
<column xsi:type="timestamp" name="created_at" on_update="false" nullable="false" default="CURRENT_TIMESTAMP"
comment="Creation Time"/>
<column xsi:type="timestamp" name="updated_at" on_update="true" nullable="false" default="0"
<column xsi:type="timestamp" name="updated_at" on_update="true" nullable="false" default="CURRENT_TIMESTAMP"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, we are not allowed to change column default value according to our Backward compatible development guide

Copy link
Member Author

@dverkade dverkade Mar 22, 2019

Choose a reason for hiding this comment

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

@sidolov how do you expect me to fix this issue? So there is a real issue with these fields, but due to self proclaimed backwards compatibility this can't be fixed.

Then we could just remove these fields as well, because they are never going to work anyway if this fix can't be accepted.

And since there is no 2.4 develop branch yet, I can't do a pull request against that branch as well. A 2.4 develop branch should have been made available right after the release of 2.3 so that new backwards incompatible changes can be done against that branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is being reviewed internally now, thanks for your help @dverkade!

@ghost ghost removed the Progress: accept label Mar 14, 2019
@sidolov
Copy link
Contributor

sidolov commented Apr 11, 2019

Hi @dverkade , @miguelbalparda recently we discussed such case with our architect and decided to accept current PR as reasonable.

We need additionally verify a few cases to make sure that the solution works properly.

Notes for QA
Please, verify the following cases with provided changes:

  • The dates are saved in GMT/proper timezone, following the best practices
  • The dates are verified when shown on front-end/ backend to be properly converted to the right timezone/locale.
  • Testing should take into account server TZ, Mysql TZ, PHP TZ, TZ set in admin and the user locale/TZ for frontend.

@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-4454 has been created to process this Pull Request

@dverkade
Copy link
Member Author

Hi @dverkade , @miguelbalparda recently we discussed such case with our architect and decided to accept current PR as reasonable.

Thanks for accepting this PR!

@soleksii
Copy link

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented Apr 29, 2019

Hi @dverkade, 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.

8 participants