Snapshot merge resolve conflicts

[PatelUtkarsh]

• [x] Add UI for resolve conflict
• [x] Add unit-test

[PatelUtkarsh]

From @miina

One thing that possibly could be useful would be to see somehow which Snapshots are merged. For example, have information on the merged Snapshot that it's merged from X & Y or show in the Snapshot that was merged sth like merged into Z. Currently, when testing with various Snapshots it was hard to understand later which Snapshots were merged. Or is this solved in some other way that I didn't notice? Or maybe this is irrelevant? Thoughts?

We don't store information of snapshots that are merged, we have information on which settings are being conflicted, but we can certainly add that data. I think it would be useful in some cases.

@westonruter thoughts?

[westonruter]

@PatelUtkarsh yes I agree. There can be an array with each setting in the snapshot that contains the list of snapshots UUIDs that served as the sources for that value, even when snapshots were merged that had the identical value. When the value is not identical, then the original values could be included as previously discussed.

[PatelUtkarsh]

even when snapshots were merged that had the identical value

Currently, If two snapshot has a conflicting setting we don't check for their values are identical or not so it will be shown as conflicting.

I will add improvement to check for same values to this PR.

@westonruter When you said to save an array of UUID in setting's data and not in post-meta, is it because if some setting gets deleted we don't need to take care of removing that UUID from post-meta?

So later we can get all unique UUIDs from settings and display somewhere in meta-box.

[westonruter]

@PatelUtkarsh I suppose the UUIDs of the origins could be stored in postmeta instead (or maybe in addition), but I think what I was getting at is that if a snapshot gets merged with additional snapshots over and over again: considering merging two snapshots that were themselves merged. If the UUIDs are attached at the setting level, then you'd be able to know which snapshots those setting values came from specifically.

[PatelUtkarsh]

@westonruter Can you check above approach? You can get the idea from test case here https://github.com/xwp/wp-customize-snapshots/blob/b67ad83608ed4025ec2ecd6d52f83cae13d7a194/tests/php/test-class-post-type.php#L853-L996

I have added following things.

• Handle merged snapshot to merge again (handle merge_conflict and merged_uuid keys)
• Add UI to display merged snapshots for current snapshot
• Check for identical values on merge conflict. (If all values are identical it will be not considered as merge conflict else it will be considered as merge conflict even if 2 of 3 values are same)

[miina]

@PatelUtkarsh Great work on the Snapshots!

Some questions / thoughts I had when trying it out:

• Would it be possible to merge the post setting? Currently when I do this: 1) Add Snapshot 1 with title changed and description changed, 2) Add Snapshot 2 with just title changed, 3) Merge these two and favor the Snapshot 2 when resolving conflicts, the description change of the Snapshot 1 gets lost.
• The Resolve conflicts is purposefully going to stay available? So it won't permanently resolve/change the Snapshot but rather you can choose which Snapshot to favor and you can change it anytime? In this case maybe the Resolve Conflicts might be confusing? Since even if you have already chosen an option it'll still show you as if you'd need to resolve conflicts. Thoughts?
• Is there some kind of logic for managing merging already Scheduled Snapshots? Or should the already scheduled Snapshots be "mergable"? Thinking that maybe it's safer if the scheduled Snapshots couldn't be merged? For example if I merge already scheduled Snapshots and then Schedule the merged Snapshot, then all 3 will be scheduled? Or it's completely the responsibility of the user to have an overview of that? Not sure :) Any thoughts on that?
• If a Snapshot has a name, maybe it's better to show the name instead of the ID in the list of This snapshot is merged from:
• One thing I saw unrelated to your PR -- the slug of Snapshot is changeable from Quick Edit, that should probably be forbidden?
• When checking the future preview with 1 Snapshot (without conflicts), I could still edit it in Customizer and change a value with clicking "Schedule", is this expected behavior? Maybe it is :)
• When checking the future preview with multiple Snapshots scheduled and when I click Customize and make some edits, it's going to create a new merged Snapshot, is this expected? However, this new Snapshot will not be Scheduled unless scheduled specifically, so it won't actually make any changes. Maybe this can be confusing? Thoughts?

[PatelUtkarsh]

@miina Thank you for testing this :) Please find the answers below.

• Currently, A post is stored as single setting with all post data in the snapshot, We are merging two or more settings so it can be either 1st snapshot's post or 2nd's can't break down further more. Part of customize-post.
• Yes, so that allows a user to re-select setting anytime, We should rename it any thoughts on another name?
• There is no logic for mergeable, We are allowing all snapshot to be able to merge discussion here https://github.com/xwp/wp-customize-snapshots/issues/67.
• Yep good catch i will update it with default signature as {Name} - {UUID}.
• Looks like you found a bug should report under issues.
• I haven't got approval on that one yet, So if there is only one snapshot we are directly previewing it right now in (#95), Maybe we should fork/duplicate it and preview it. I thought because it is just previewing we can skip extra steps of duplication.
• Yes, so when you preview from dashboard it creates new merge snapshot with auto-draft status(merging all future snapshots comparing to date). If you start editing it, it will put that in draft status and will allow you to edit.

[miina]

@PatelUtkarsh

Yes, so that allows a user to re-select setting anytime, We should rename it any thoughts on another name?

Maybe "Merge conflicts, multiple choices available." And / or a link "Change selection"? Or just "Change merge conflict selection"?

There is no logic for mergeable, We are allowing all snapshot to be able to merge discussion here #67.

Okay :+1: For me it did feel like it might get out of hand when merging different Scheduled Snapshots and then Scheduling those. Maybe it would be a good idea to somehow let the user know that the other Snapshots are still scheduled? E.g. in This snapshot is merged from: in addition to the name and link also display (Scheduled) in case it's scheduled? Thoughts?

Yes, so when you preview from dashboard it creates new merge snapshot with auto-draft status(merging all future snapshots comparing to date). If you start editing it, it will put that in draft status and will allow you to edit.

Will move this to #95.

[PatelUtkarsh]

@miina

Maybe "Merge conflicts, multiple choices available." And / or a link "Change selection"? Or just "Change merge conflict selection"?

Maybe "Change Merge Selection" ?

[PatelUtkarsh]

@miina

Okay 👍 For me it did feel like it might get out of hand when merging different Scheduled Snapshots and then Scheduling those. Maybe it would be a good idea to somehow let the user know that the other Snapshots are still scheduled? E.g. in This snapshot is merged from: in addition to the name and link also display (Scheduled) in case it's scheduled? Thoughts?

Yes we can add that, would that be only for scheduled or we should display all post-status? I think only schedule makes sense.

[miina]

@PatelUtkarsh Agreed that only "Scheduled" is relevant to display.

[coveralls]

Coverage increased (+2.08%) to 92.67% when pulling 411cc862d30acac8c3221ab84cd518b8fd7756c9 on feature/resolve-conflict into 8a253fdb85d5a64ea1e5ec8fb555966055b4a051 on develop.

[coveralls]

Coverage decreased (-11.7%) to 78.865% when pulling 0ade9a623bb849f967b34a68048681c745680edd on feature/resolve-conflict into 629033dd3704945aa78e72986441425925c1b1a2 on develop.

[coveralls]

Coverage decreased (-11.6%) to 79.015% when pulling 4a06f3ddf3672439758b769f795af354a87d5d93 on feature/resolve-conflict into 629033dd3704945aa78e72986441425925c1b1a2 on develop.

[coveralls]

Coverage decreased (-11.6%) to 79.015% when pulling b44b30810b106465806b0678deab5373b63ca7cb on feature/resolve-conflict into 629033dd3704945aa78e72986441425925c1b1a2 on develop.

[coveralls]

Coverage decreased (-11.6%) to 79.015% when pulling f33fc314cbe2b3d96cb0548db97be50f8b73a720 on feature/resolve-conflict into 629033dd3704945aa78e72986441425925c1b1a2 on develop.

[valendesigns]

@PatelUtkarsh Please merge develop into this branch so I can review. Cheers!

[coveralls]

Coverage increased (+2.0%) to 76.871% when pulling 11c068300371e5560f4ee4fff03c01e3e5da80f1 on feature/resolve-conflict into 1784a74e739ae47f91193db47e5a8ee5083bbc15 on develop.

[PatelUtkarsh]

@sayedtaqui I am adding dependency on #71 as it will have conflicts with it when it merges and there are some todos that we will need to fix when #71 gets merged and we can use some style for admin area from it.

[coveralls]

Coverage increased (+2.9%) to 83.045% when pulling e69d4a884a0aa7141b197d7f14d6070e39809c2a on feature/resolve-conflict into 91b082732499d59fbbe9e16b872577f6b8903960 on develop.

[coveralls]

Coverage increased (+2.4%) to 82.591% when pulling a2f686e90c5a1595fba208e98e1de1ae6ae82834 on feature/resolve-conflict into 91b082732499d59fbbe9e16b872577f6b8903960 on develop.

[coveralls]

Coverage increased (+2.7%) to 82.409% when pulling ab567f31c379d9badcc6054800edfe693b16911e on feature/resolve-conflict into 68e99907f89aba7d047d0f0ec771ff16169e9bd1 on develop.

[coveralls]

Coverage increased (+2.7%) to 82.409% when pulling acb3b6c919cae1d632e6981a115780af9303e870 on feature/resolve-conflict into 68e99907f89aba7d047d0f0ec771ff16169e9bd1 on develop.

[coveralls]

Coverage increased (+2.5%) to 82.154% when pulling 35184c89effface4064647079227988e4d771add on feature/resolve-conflict into 68e99907f89aba7d047d0f0ec771ff16169e9bd1 on develop.