Skip to content

Conversation

@osrecio
Copy link
Member

@osrecio osrecio commented Dec 11, 2017

Set Current Date Time to update_time field

Description

add current date time to 'update_time' in _beforeSave method for Page and Block

Fixed Issues (if relevant)

  1. when saving a page in magento 2.2.1, 'Modified' date field is not getting updated #12625: when saving a page in magento 2.2.1, 'Modified' date field is not getting updated

Manual testing scenarios

  1. Login to magento admin
  2. Go to Content > Pages
  3. In Action column, click select > edit
  4. Make changes to that page and click 'Save Page'

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)

@osrecio osrecio changed the title #12625 Add Current Date to update_time Field Block and Cms #12625 Add Current Date to update_time Field for Block and Pages Dec 11, 2017
@magento-engcom-team magento-engcom-team added Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Dec 11, 2017
@osrecio osrecio changed the title #12625 Add Current Date to update_time Field for Block and Pages #12625: Add Current Date to update_time Field for Block and Pages Dec 11, 2017
@mzeis mzeis self-assigned this Dec 11, 2017
@mzeis mzeis added this to the December 2017 milestone Dec 11, 2017
@mzeis
Copy link
Contributor

mzeis commented Dec 11, 2017

Hi @osrecio,

thanks for your pull request! Please can you have a look at the failing integration test?

@osrecio
Copy link
Member Author

osrecio commented Dec 11, 2017

@mzeis Adapted Tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use framework implementation for date value \Magento\Framework\Stdlib\DateTime\DateTime::date

@sidolov sidolov self-assigned this Dec 14, 2017
@osrecio osrecio force-pushed the PR#12625_2.2 branch 2 times, most recently from dd2e65f to 00c3c28 Compare December 14, 2017 14:26
@osrecio
Copy link
Member Author

osrecio commented Dec 14, 2017

@sidolov Changes made, and added integration tests for Cms Block 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to get the object manager in the setUp method (example).

@osrecio
Copy link
Member Author

osrecio commented Dec 17, 2017

@mzeis Added ObjectManager to setUp.

I copied test from dev/tests/integration/testsuite/Magento/Cms/Model/PageTest.php

@osrecio
Copy link
Member Author

osrecio commented Dec 18, 2017

@sidolov I followed your suggestions and I adapted my PR.

@mzeis
Copy link
Contributor

mzeis commented Dec 20, 2017

@osrecio When comparing the changes with #12625, I noticed that the implementation changes are the same but the tests are carried out differently. Would it make sense to adjust the integration tests here to make them equal to 2.3-develop?

@magento-engcom-team magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Dec 20, 2017
@osrecio
Copy link
Member Author

osrecio commented Dec 21, 2017

@mzeis Are you referring to the tests changed in branch 2.3-develop of my other PR?

These test: BlockTest.php , PageTest.php

Thanks!

@mzeis
Copy link
Contributor

mzeis commented Dec 21, 2017

@osrecio Yes exactly. Since Stanislav approved your changes in #12637 I thought it might be a good idea to keep the code between 2.2 and 2.3 as similar as possible.

@osrecio
Copy link
Member Author

osrecio commented Dec 21, 2017

I will work to make these changes, Thanks @mzeis

@osrecio
Copy link
Member Author

osrecio commented Dec 21, 2017

Hi @mzeis I adapted BlockTest.php like 2.3-develop in PageTest.php nothing to do because other methods in tests covering new funcionality not included in 2.2-develop.

@sidolov
Copy link
Contributor

sidolov commented Dec 21, 2017

@osrecio, please, check failed static tests

@osrecio
Copy link
Member Author

osrecio commented Dec 21, 2017

@sidolov Fixed Static Tests, the pre-commit deleted by me the reference in namespace 😄

@mzeis
Copy link
Contributor

mzeis commented Dec 23, 2017

@osrecio Many thanks for your contribution. It has been merged. Happy holidays! 🎄

@osrecio
Copy link
Member Author

osrecio commented Dec 23, 2017

Thanks @mzeis , Happy holidays and happy new year!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Progress: accept Release Line: 2.2 Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants