bug: propagate deletion of parent PIDs properly #1864
Conversation
carlinmack
changed the title
carlinmack
force-pushed
the
propagate-del
branch
from
c599d45
to
d83ceaa
Compare
|
|
||
| @shared_task(ignore_result=True) | ||
| def cleanup_parent_pids(record): | ||
| """Clean up parent PIDs which could not be deleted by the service.""" |
There was a problem hiding this comment.
Very minor: the comment assumes that we are calling this task only from a specific service method.
Could you improve it to explain what is happening here instead?
Looks like that it will discard all drafts if there is no other published record?
What happens if there are no drafts? (well, probably the parent should not exist)?
There was a problem hiding this comment.
Noted, however just fyi that this doesn't work on drafts
There was a problem hiding this comment.
Noted, however just fyi that this doesn't work on drafts
OK, it makes sense, can you make it explicit it then? e.g. assert that it is a record, and fail if it is a draft?
I see that it is handled in the code ;)
carlinmack
force-pushed
the
propagate-del
branch
4 times, most recently
from
cf7d61e
to
38f5e94
Compare
| def cleanup_parent_pids(recid): | ||
| """Clean up parent PIDs.""" | ||
| record_cls = current_rdm_records.records_service | ||
| record = record_cls.pid.resolve(recid) |
There was a problem hiding this comment.
I understand that this task is initiated when the recid is deleted right? If so, wouldn't that resolve fail?
carlinmack
force-pushed
the
propagate-del
branch
from
7c03d27
to
a7804f6
Compare
carlinmack
force-pushed
the
propagate-del
branch
from
a7804f6
to
dca0674
Compare
| # We're sending a task in case there is a race condition with two | ||
| # versions being deleted at the same time to ensure that we have | ||
| # consistent database state | ||
| self.uow.register(TaskOp(cleanup_parent_pids, record["id"])) |
There was a problem hiding this comment.
It looks like that the if above is copy/pasted in the celery task.
Does it make sense to completely remove the if condition here and simply call:
self.uow.register(TaskOp(cleanup_parent_pids, record["id"]))?
This is a bit confusing.
|
Can you please rebase after @alejandromumo fixed the tests? It should solve the CI failure. |
❤️ Thank you for your contribution!
Relies on #1810 being merged first
Close #1863
Description
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: