Problem/Motivation

This initially started in #2916939: Add completion time to orders, document changed time behaviour and that was changed to fix the completion time and add documentation for the changed time problem.

From the other issue:
To find out where the time is changed I had a look at the commerce pre and post save and events and don't see anything helpful. And didn't track it down during a brief debug session. For reference I checked the d6 and d7 Core Node tests, the d6 one does not test the time changed but the d7 one does. From that one would expect that the changed time would be what was migrated not the time of the migration.

Anyone know where the change time is modified?

Found the problem. In Order::preSave() when the order is in draft and there is no refresh state then it is set to REFRESH_ON_SAVE and the OrderRefresh::refresh() modifies the changed and completed times. Setting the refresh state to 'refresh_skip' seems to prevent that. Will that cause any problems? Don't know yet.

See order and refresh process documentation.

Proposed resolution

For draft orders set the refresh state to 'refresh_skip

Remaining tasks

Patch
Review
Commit

CommentFileSizeAuthor
#13 interdiff.txt984 bytesquietone
#13 2948601-13.patch4.08 KBquietone
#6 interdiff.txt2.64 KBquietone
#6 2948601-6.patch4.04 KBquietone
#5 2948601-4.patch1.41 KBquietone
#4 interdiff.txt413 bytesquietone
#2 2948601-2.patch1.39 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 2948601-2.patch1.39 KB

When the order is not complete then it is set to refresh on save and the refresh modifies the changed and completed times. Setting the refresh state to 'refresh_skip' seems to prevent that.

Status: Needs review » Needs work

The last submitted patch, 2: 2948601-2.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt413 bytes

Add bypass: true to the new static map.

quietone’s picture

quietone commented
StatusFileSize
new 2948601-4.patch1.41 KB

Forgot the patch

quietone’s picture

quietone commented
StatusFileSize
new 2948601-6.patch4.04 KB
new interdiff.txt2.64 KB

Now update ubercart order migration. Also removed the checks on NULL for the completed and changed time tests so that those assertions always execute in assertOrder().

quietone’s picture

quietone commented
Issue summary: View changes

The next thing is to check with Commerce folks if migrating with a refresh_state of skip_refresh has any unwanted side effects.

quietone’s picture

quietone commented
Issue summary: View changes

Talked to mglaman on Slack and he said that there is not problem with setting the refresh to skip. The next time the order is loaded it will be updated and the changed time will be modified.

That means it could be argued that this patch isn't necessary, that the order changed time is being correctly modified by Commerce 2 processes. If that were the case then documentation should be added to the migration yml files and well as the Commerce Migrate docs explaining this behavior. On the other hand, migration is about keeping the data the same. Right now I lean to using this patch but can be convinced otherwise.

See the Commerce 2 documentation for order and refresh process.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

+1 on refresh_state=skip_refresh. It makes tests easier.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work 
+++ b/modules/commerce/migrations/d7_commerce_order.yml
@@ -32,6 +32,16 @@ process:
+  data/refresh_state:
+    plugin: static_map

Can this just be a statics/refresh_skip in the source and direct mapping? Why this extra work?

quietone’s picture

quietone commented
Status: Needs work » Needs review

I prefer exposing what is going on in the migration yml file. I reckon it is the first place someone looks to get an idea of what the migration is trying to do and the more the process is visible there, the less they have to look elsewhere. In this case, since the mapping depends on a status field that is configurable on the source it makes sense to me to put it here so it can be easily modified for custom situations. Of course, that is also assuming that changing the migration yml is easier that changing a source plugin. That is an assumption I think it true.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

re #11: Not in the plugin, in the source section of the yml.

quietone’s picture

quietone commented
StatusFileSize
new 2948601-13.patch4.08 KB
new interdiff.txt984 bytes

Ah, yes, sorry. I got sidetracked with the states.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Reviewed & tested by the community

All feedback addressed.

  • heddn committed c78f2d6 on 8.x-2.x authored by quietone 
    Issue #2948601 by quietone, heddn: Fix that order changed times are not...
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.