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 backend creationdate bug #465

Closed
wants to merge 1 commit into from
Closed

Conversation

seansan
Copy link
Contributor

@seansan seansan commented Mar 9, 2018

We dont want system time in backend.

https://magento.stackexchange.com/questions/38143/customer-created-logged-in-timestamps-in-admin

Discovered whilst testing for GDPR: when customer asks "when" did I subscribe our data is always 1 hour off

We dont want system time in backend. 

https://magento.stackexchange.com/questions/38143/customer-created-logged-in-timestamps-in-admin

Discovered whilst testing for GDPR: when customer asks "when" did I subscribe our data is always 1 hour off
@@ -78,8 +78,12 @@ public function getCustomerLog()
*/
public function getCreateDate()
{
return $this->_getCoreHelper()->formatDate($this->getCustomer()->getCreatedAt(),
Mage_Core_Model_Locale::FORMAT_TYPE_MEDIUM, true);
$date = Mage::app()->getLocale()->storeDate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation level is off on this whole block.

Copy link
Contributor

@LeeSaferite LeeSaferite Mar 9, 2018

Choose a reason for hiding this comment

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

Generally speaking I disagree with this PR since it's not fixing a bug but instead changing behavior to what you expect. This method is meant to return a date formatted for the currently selected timezone as it's stored in the DB (which should be UTC I would hope) . If you notice, the very next method is getStoreCreateDate() and it does exactly what you want. If you think admin should be displaying the store date then instead why not edit the template to use the correct method? Even better would be a new bool config path and then check that in the template to have both behaviors. You could leave the default as the old behavior and use the flag to change to using store times instead.

@seansan
Copy link
Contributor Author

seansan commented Mar 9, 2018 via email

@seansan
Copy link
Contributor Author

seansan commented Mar 9, 2018

Looks like the template already accounts for difference for timezone vs time

But in our case the 2 are the same which is strange because it should not be (there is 1 hour offset)

image

@seansan
Copy link
Contributor Author

seansan commented Mar 9, 2018

See #466

@seansan seansan closed this Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants