Dangling relationships after removing a working copy

[frapell]

An issue exists in this product, so when you either cancel a check-out or do a check-in, the relationship between the baseline and the working copy is not removed, but instead flagged as "broken". I am not sure what was the rationale behind this implementation but it results in a broken functionality:

1. After creating a checkout of an item, you can properly see a notification on top, between the baseline and the working copy:

Baseline

Working Copy

2. You can either cancel the check-out or do a check-in
3. If you now create another checkout, the notifications now look like this:

Baseline

Working Copy

The reason for this is because the relationship between them, instead of being removed, is marked as "broken"

plone.app.iterate/plone/app/iterate/dexterity/copier.py

Lines 113 to 117 in 6c7af3b

	def _deleteWorkingCopyRelation(self):
	# delete the wc reference keeping a reference to it for its annotations
	relation = self._get_relation_to_baseline()
	relation.broken(relation.to_path)
	return relation

Then, when the viewlet tries to find the relationship at

plone.app.iterate/plone/app/iterate/dexterity/utils.py

Lines 30 to 35 in 6c7af3b

	def get_checkout_relation(context):
	relations = get_relations(context)
	if len(relations) > 0:
	return relations[0]
	else:
	return None

it will find several (broken) relationships, and returns the first one.

I guess one way of fixing this would be to change that get_checkout_relation so it will iterate over all relationships and at least return the first one that is not broken, however I do not see the reason nor the value on keeping relationships in a broken state, so I think the proper way to fix it is to actually remove the relationship from the catalog altogether.

[ewohnlich]

I just came across this exact issue and indeed my suggestion was going to be to modify get_checkout_relation to return the first non-broken relation instead of just assuming the first item is the intended one. Even if this package handles garbage collection better it's probably worth explicitly doing this check for any edge cases.

[frapell]

@ewohnlich Well, there are 2 things with that solution:

1. I don't know where garbage collection is done... I believe these broken relationships are stuck in the catalog forever
2. I don't think it is a good implementation, because to me, the state of the system should be consistent:
   a. You have an empty catalog
   b. You create a working copy
   c. You have now a relationship in the catalog
   d. You check-in or cancel the working copy

At this point, you are at the same place you were in step "a" but the catalog is in a different state, where a broken relationship exists... I have been tied up with other things and wasn't able to finish my PR here #78 I hope to be able to do so this weekend... And, btw, if you want to help me over there, you are welcome :)

[ewohnlich]

The strange thing to me is that this behavior seemed to be working in a previous version of Plone. We came across a bug where if the user deleted a wc, instead of canceling the checkout, it would cause this problem. But canceling the checkout worked as expected, so the solution was to add an event listener on deletion to call notify(CancelCheckoutEvent(ob, baseline)) if the deleted item was a working copy. Obviously that doesn't work now because canceling the checkout is itself broken. I wonder if perhaps some event listener was doing cleanup and this was altered in the recent past.

[frapell]

@ewohnlich yup, you even opened a bug 2 years ago about it #63 we also fixed that by implementing the same event subscriber... I will try to fix that too in this package in another PR

[ewohnlich]

@frapell can you explain why you think _deleteWorkingCopyRelation is involved? As far as I can tell that is only trigged on check in (merging) and not on cancelation.

[frapell]

@ewohnlich Yes, that's correct, if you check my fix in https://github.com/plone/plone.app.iterate/pull/78/files you'll see that there are 2 places where this needs to be addressed, on cancelling a checkout and on checking in... in both cases the working copy is deleted, and so the relationship should no longer exist...

There are some possible edge cases as Maurits mentioned in #78 (comment) that needs to be tested, in order to validate there are no side effects by removing the relationship, instead of marking it as broken