Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix project transfer failure in some circumstances #5135

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

noliveleger
Copy link
Contributor

@noliveleger noliveleger commented Sep 30, 2024

Description

Fix an error when project is not transferred entirely to the new owner and data is missing under the account of the new owner.
Project returns the correct number of submissions, but the table view only returns a subset of it.

Notes

A new management command has been added to resume broken transfers done after 2.024.25 release has been deployed: resume_failed_transfers_2_024_25_fix

The main problem was a race condition between uWSGI process and Celery process. The PR ensures that Celery tasks are triggered IFF data in transaction is saved in DB, thanks to transaction.on_commit hook.

Moreover, media_file field of Attachment model is changed to ExtendedField like it is in its shadow model counterpart (i.e.: KobocatAttachment). It provides the method move() which is required by transfer asynchronous tasks. See https://github.com/kobotoolbox/kpi/blob/834ec518a9487dcfacd51e2476f1ad6fafda041c/kpi/fields/file.py for more details.

@noliveleger noliveleger added CRITICAL Red alert! Fix me ASAP! bug-fix Back end labels Sep 30, 2024
@noliveleger noliveleger self-assigned this Sep 30, 2024
@noliveleger noliveleger marked this pull request as ready for review October 1, 2024 16:52
Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One non-blocking question

@@ -1,4 +1,3 @@
# coding: utf-8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was adding the linter to CI, I came across this rule. I did not want to enforce it in the CI but I decided to remove them manually when I was touching a file.

@noliveleger noliveleger merged commit 2ff37e7 into release/2.024.25 Oct 1, 2024
4 checks passed
@noliveleger noliveleger deleted the fix-transfer-ownership-race-condition branch October 1, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Back end bug-fix CRITICAL Red alert! Fix me ASAP!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants