Attempt to fix undo author merges #820
Conversation
| # the code below checks if authors are currently redirects in ctx.site | ||
| # but if the redirect is incorrect and being reverted in this changeset, | ||
| # the code below will update all the books to have the incorrect target | ||
| # author... |
There was a problem hiding this comment.
@mekarpeles can you check this logic here, I think this whole method is unnecessary and is preventing undos from working, but am worried I'm missing something. If this hook is to do anything, it should revert all author changes first, then allow the books to be reverted, but it is not set up to work like that.
I think the site will prevent work saves that reference a redirect author, so an Author merge undo will need to first revert the author redirects and save, then undo the works/editions to the previous correct (now not a redirect) author.
|
@hornc updates on this PR? |
this method followed the previous works' author redirects, which we are trying to undo, and updated the doc's author to that target. I think we just want a simple undo to latest rev - 1 for all docs.
hornc
changed the title
|
@mekarpeles I have updated this PR, it needs a review. I have tested a relatively simple author merge undo locally, but it is possible I have missed some more complex merge undo scenario. I've noticed solr is not updating after the undo, but I believe that is caused by #927 |
| @@ -822,17 +822,6 @@ class MergeAuthors(Changeset): | |||
| def can_undo(self): | |||
| return self.get_undo_changeset() is None | |||
|
|
|||
| def process_docs_before_undo(self, docs): | |||
There was a problem hiding this comment.
Is this called anywhere? Do we need to remove calls to it?
There was a problem hiding this comment.
@mek It is a hook called by the parent class, but I don't think it does anything useful, in fact following author redirects here is the worst thing it could do. The operation is to revert an author merge, and this looks at the undo changeset, and replaces the original author (the one we are trying to revert back to) with the current target of the redirect, which is the incorrect author we are trying to revert away from. This is why author merges never seemed to work, this code ensured that the incorrect author stayed attached to all the works.
I think it has been expressed by other too (@cdrini and @tfmorris ?) that the revert of a changeset should be a simple undo, with no extra unexpected logic.
|
LGTM |
In the process of undoing an author merge @tfmorris and I noticed that authors on edition records were being stripped, which left orphaned editions (i.e. editions with no works) without an author too. This is bad as it makes it harder to group them correctly.
While ideally authors should only be stored at the work level, reverting undo is not the place to make this happen. Also, currently it is very helpful to have author data on orphaned editions to help group them under existing works.
Undo should simply revert the record to the previous version, with no other modifications.