When a replication occurs the following steps are taken.
- Check for conflicts (i.e. existing conflicts)
- Update the source workspace (e.g. pull upstream into active)
- Check for conflicts (i.e. conflicts discovered after updating)
- Deploy changes to the target workspace (e.g. push active to upstream)
The problem is that nothing is done if conflicts exist, thus the user unknowingly may push conflicts to an upstream workspace, such as the Live workspace.
Suggested Resolution
The suggested resolution is to allow the user to decide whether or not to proceed with pushing upstream if there are conflicts. The revised steps in bold should look like:
- Check for conflicts (i.e. existing conflicts)
- Abort replication if there are conflicts and "abort on conflicts" flag is true (i.e. return failed replication log)
- Update the source workspace (e.g. pull upstream into active)
- Check for conflicts (i.e. conflicts discovered after updating)
- Abort replication if there are conflicts and "abort on conflicts" flag is true (i.e. return failed replication log)
- Deploy changes to the target workspace (e.g. push active to upstream)
Consequently, this means the caller of ReplicatorManager needs to handle the failed replication log appropriately.
Part 1: Revise ReplicatorManager
For ReplicatorManager this means giving control back to the caller of ReplicatorManager::replicate in the event of conflicts. Specifically, this looks like:
- Add a flag to the ReplicatorManager to tell it to proceed with replicating upstream after an update even if there are conflicts.
- If there are conflicts in ReplicatorManager::replicate and the abort flag is set to abort on conflicts, return a failed replication log and do *not* proceed with replicating upstream, i.e. abort just after the "update" call.
Note: if it is possible to have the "abort on conflicts" flag keep track of a sequence index this would allow handling the case where: (a) a user attempts replication, (b) it aborts due to conflicts, (c) the user reviews the conflicts, (d) the user attempts replication again, but there were new conflicts introduced since step "b" and they unknowingly replicate those conflicts upstream.
Part 2: Revise Callers of ReplicatorManager
For callers of ReplicatorManager, they will need to know that when a failed replication log is returned they need to handle it properly. I don't believe there currently is handling for failure, so we will want to introduce handling for conflicts as well as a general case (assuming it makes sense to also introduce that now).
These include:
- Drupal\workspace\Plugin\RulesAction\ReplicateContent
- Drupal\workspace\EventSubscriber\WorkbenchModerationSubscriber
- Drupal\deploy\Entity\Form\ReplicationForm
- Drupal\deploy\Form\ReplicationActionForm
Here is how we will handle each case in this ticket:
- Drupal\workspace\Plugin\RulesAction\ReplicateContent: Deal with later; Find a way to cancel the Rules action
- Drupal\workspace\EventSubscriber\WorkbenchModerationSubscriber: Use hook entity type alter to allow the user to pass the "override" flag if there are conflicts
- Drupal\deploy\Entity\Form\ReplicationForm: Deal with later
- Drupal\deploy\Form\ReplicationActionForm: Deal with later
DONE:
- Show a message to the user if there are conflicts and given them the option of proceeding with replication even if there are conflicts
- Show a message to the user when replication fails due to conflicts
- Allow the user to see a list of conflicts in a Workspace
- If a replication fails, the WorkbenchModerationSubscriber will need to revert back the state.
- If the list of conflicted revisions does not have a link to the entity, so there issue where the link would go to an entity that doesnt exist on the active workspace
TODO:
- Write tests
- Create tickets for the "deal with later" items like handling situations like when Rules is used or Deploy
Comment | File | Size | Author |
---|---|---|---|
#65 | interdiff.txt | 3.76 KB | jeqq |
#65 | workspace_should_report-2791789-65.patch | 41.17 KB | jeqq |
#59 | interdiff.txt | 517 bytes | jeqq |
#59 | workspace_should_report-2791789-59.patch | 41.05 KB | jeqq |
#54 | interdiff.txt | 4.19 KB | jeqq |
Comments
Comment #2
phenaproximaComment #3
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedUpdated description to reflect specifics of next steps and clarify the problem.
Note: instead of adding the "abort on conflicts" flag to the ReplicationTask, I'm adding it to the ReplicatorManager since this is a problem specific to the ReplicatorManager implementation--particularly, not all ReplicatorInterfaces will do an "update" before a "replicate".
Comment #4
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedComment #5
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedComment #6
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedThis is a sketch of what needs to happen in the ReplicationManager.
Next, we need to pull $is_aborted_on_conflict flag out to some shared state and then modify the callers of ReplicationManager to be able to change that flag based on user input, i.e. if the user does want to push conflict.s
Comment #7
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedComment #8
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedComment #9
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedThis patch is very rough but at least gives a proof of concept of what the user might see. I've attached screenshots that show what the user would see if a Workspace has no conflicts, when there are conflicts, and then what the revision list currently looks like.
On a high level, here's what this patch does:
Next steps:
Comment #10
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedMore specific next steps:
Comment #11
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedHere are the latest changes:
Comment #12
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedHere are the latest changes:
So, the only part addressed here is item 6. This patch *should* work in theory, but for some reason the following is taking place:
You'll see a success message, which makes sense since the moderation state of the new workspace successfully transitioned. However, it doesn't appear the abort step is being run, which is very confusing (i.e. line 85 of ReplicationManager doesn't trigger).
So, two things need done:
1. If a replication fails, the WorkbenchModerationSubscriber will need to revert back the state (is this possible?).
2. Figure out why the replication isn't aborting if there are conflicts.
This seems like a perfect case for a test!
Note: I've moved the list of TODOs to the description.
Comment #13
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions for Acquia commentedWhoops! Forgot the patch.
Comment #14
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedSo, it turns out the check for the replication log status was incorrect. I identified the same problem with Deploy #2809325: Correctly access replication log status value.
So, this patch fixes that part. Still need to do tasks 4-8 in the description.
Comment #15
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedThis patch uses a list builder for the list of conflicts.
Comment #16
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedUpdating description to clarify whats next.
Comment #17
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedThis patch takes care of reverting the workflow state on failed replication, however it uses a static variable which is... not ideal.
I've added a TODO which is to reconcile the WorkspaceForm and WorkbenchModerationSubscriber messages (see screenshot).
Two more items to go plus writing tests. I'm not sure I will get to these items, so if someone picks them up message me and I can give some clarity on where to get started.
Comment #18
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedComment #19
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedHere is a screenshot showing the case where multiple messages show at the same time:
Comment #20
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedUpdating the TODO list, which contains all the summarized info from the comments.
Comment #21
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedRegarding the problem of the conflicts listing being linked out to entities that may not exist on the current workspace, I took a look this morning and see the following options:
I like the first option since it keeps it simple (see screenshot). However, the second option may be more useful to the user. The problem with the second option is we would likely have to write a form of somekind to handle the switching of the workspace and redirect to the entity. This may pose a usability problem of not being able to easily get back to the conflicts list. Perhaps there is a view that displays the revision regardless of which workspace is active?
I could use some input on this as well as how we might consolidate the messages (see screenshot).
Comment #22
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedAlright, I believe all the major issues are tackled aside from writing tests.
Here are screenshots of what it looks like:
Setting this as ready to review to get some eyes on it.
Comment #23
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedComment #24
phenaproximaThis verbiage seems a little intimidating and obscure for the average user, even a techy one :) I would recommend something like this:
"There are [link to conflict list]3 conflict(s) with the [upstream] workspace[/link]. Pushing changes to [upstream] may result in unexpected behavior or data loss, and cannot be undone. Please proceed with caution."
Should be !isset() or === because any falsy value will == NULL.
Comment #25
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedThose two items are taken care of in this patch.
I also remove the conflict list template file that isn't used, and modify the list builder to use entity keys.
Comment #26
jeqqThis is a quick code review, will do a more detailed review of the functionality later.
Variable
$row
is defined twice.Remove this service if it is not used or inject the
EntityTypeManager
.Use
Drupal\Core\Url
insted.Add
@return array
.Use
Drupal\Core\Entity\EntityInterface::toUrl()
instead,Drupal\Core\Entity\EntityInterface::urlInfo()
is deprecated.Comment #27
jeqqComment #28
phenaproximaAdded support for the event stuff over in Replication: #2814055: Allow modules to react to replication events.
Comment #29
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedThis patch addresses some of the code cleanup items.
Comment #30
jeqqI would insist on replacing deprecated urlInfo() and url() with
\Drupal\Core\Entity\EntityInterface::toUrl
I've done some manual testing. Seems that changes are pulled when a conflict occurs (but they shouldn't). Steps to reproduce:
1. Enable Deploy module
2. Create a node on Live
3. Deploy to Stage
4. Edit the node on Live
5. Edit the node on Stage
6. Deploy from Live to Stage
An error message will be displayed:
Deployment error
. On live workspace now I can see changes made on Stage workspace - that means that changes were not pushed from Live to stage but they were pulled from Stage to Live.On Live workspace configuration page I see the conflict error message:
There are 1 conflict(s) with the Stage workspace. Pushing changes to Stage may result in unexpected behavior or data loss, and cannot be undone. Please proceed with caution.
, on Stage workspace configuration page:There are no conflicts.
Now, if I select
No, go ahead and push any conflicts to the upstream.
on Live and do deploy, it won't work because the entity has been modified with changes from Stage.I think we need tests for this.
Comment #31
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedI completely agree--I used the wrong method. This patch uses toUrl.
I agree that tests on this are a high priority given its complexity.
The issue described above with Deploy module is expected. Essentially what is happening is the default behavior of replication has changed by this patch from "don't abort on conflicts" to "abort on conflicts."
In the spirit of not altering behavior of other modules (i.e. deploy) I'm going to change the default back. This is a straightforward change and doesn't prevent us from still meeting the objective of this ticket.
Comment #32
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedThis patch sets the default value of "workspace_is_aborted_on_conflicts" to FALSE. This preserves the prior behavior of replicating conflicts upstream.
IMPORTANT! Any module calling this should be updated to handle this flag and by default set it to TRUE so that conflicts are not pushed upstream without the user's consent.
Comment #33
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedForgot to set to needs review.
Comment #34
phenaproximaFixed a one-character typo that broke the event dispatching stuff.
Comment #35
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedHere's the interdiff.
Comment #36
jeqqA typo here.
Let's not check the type, use just "==" because sometimes the value can be "1" for $response->get('ok')->value.
@josephdpurcell Is there a way to detect the conflict before setting the success replication message in
UdateForm::submitForm()
?. At the moment if we have a conflict and try to update the workspace, it shows the success message:Stage has been updated with content from Live.
but actually it doesn't replicate the entity because of the conflict. I can see the conflict message only if I go to workspace edit page. This looks wrong in my opinion from UX perspective. The solution would be to display the conflict message with the link to the workspace edit page, where the user can select the solution.Comment #37
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedComment #38
jeqq@josephdpurcell This still needs tests. Do you have time to implement tests for it? If not I'll work on this.
Comment #39
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedUnfortunately I do not have time for tests today. I may be able to pitch in on this later this week.
In the back of my mind for testing, I considered one scenario that involved taking a few different entity types (menu items, block content, taxonomy terms, nodes), creating a conflict between workspaces, and ensuring their labels show up in the workspace's conflict list. It seemed like a pretty straightforward high-level test to write.
Comment #40
jeqqAdded a test and fixed bugs discovered while implementing itt. The test implements the use-case described by @josephdpurcell in #39.
Comment #41
jeqqReroll.
Comment #45
jeqqFix after reroll.
Comment #46
jeqqReport conflict from the correct workspace and fix the test.
Comment #50
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedAnd this is why testing is important! Thank you so much @jeqq, that is an important find.
Comment #54
jeqqComment #55
jeqqComment #59
jeqqMinor fix for the previous patch.
Comment #63
jeqqThe test is failing because on the conflicts list page (for Live workspace), after trying to replicate content from Live to Stage(we have 3 conflicts - the correct number of conflicts), labels are not those expected. The expected labels should be those created on Live workspace but here we get, randomly, labels from Live or Stage. I'm not sure if this is the expected behaviour.
@josephdpurcell What do you think about this, maybe this is how it was supposed to work?
Comment #64
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commented@jeqq, interesting. My initial reaction is the "randomness" would be caused by the conflict resolution logic, but given that ConflictListBuilder::load is querying for a specific revision I'm not sure that logic applies.
For reference, let's clarify what the conflict resolution logic looks like so that we know if we're just querying for the entity we know what logic is followed:
This is of course simplified.
Now, looking at ConflictListBuilder::load, I don't think it would use the logic above since it's not loading the entity by ID:
We would have to look at the conflict tracking logic (link) to see what $rev_info['revision_id'] would be stored as.
Conclusion
All that aside, here is what I think might be a path forward:
Keep the Label of the entity consistent between revisions (i.e. let Stage and Live always have the same Label of the entity) so that we have a consistent identifier of the entity that the user will see.
Use the Body field as the variant, i.e. freely edit the Body field of any revisions to generate conflicts.
Have the test validate the Labels we expect to see on the conflicts list.
I hope these thoughts are helpful.
Comment #65
jeqqI've modified the test, now labels remain the same between workspaces. Is changed the body field for the node, descriptions for taxonomy term and menu link content (as @josephdpurcell proposed in #64).
Comment #67
jeqq