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(trashBin): ensure the project is fully restored from the admin interface TASK-1645 #5563

Open
wants to merge 2 commits into
base: release/2.024.36
Choose a base branch
from

Conversation

noliveleger
Copy link
Contributor

@noliveleger noliveleger commented Feb 28, 2025

📣 Summary

Fixed an issue where projects were not being completely restored from the trash.

📖 Description

A bug prevented projects from being fully restored after being moved to the trash, leading to missing or incomplete data upon recovery.

💭 Notes

Some unittests were apparently missing for this part, this PR adds two new tests. Moreover, part of kpi#5514 has been backported to this release.

👀 Preview steps

Bug template:

  1. ℹ️ have a non-admin account and a project
  2. delete the project
  3. go the admin interface and put back the project from the trash bin
  4. from the shell run this snippet
asset = Asset.objects.get(uid='<asset_uid>')
xform = XForm.all_objects.get(kpi_asset_uid=asset.uid)
print(xform.pending_delete)
  1. 🔴 [on release branch] (from the shell) notice the output is True

This snippet should raise an exception

>>> asset.deployment.xform
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/srv/src/kpi/kpi/deployment_backends/openrosa_backend.py", line 1229, in xform
    raise MissingXFormException
kpi.exceptions.MissingXFormException
  1. 🟢 [on PR] (from the shell) notice the output is False

This snippet shouldn't raise an exception

>>> asset.deployment.xform
<XForm "id_string">

@noliveleger noliveleger requested a review from jnm as a code owner February 28, 2025 09:35
@noliveleger noliveleger self-assigned this Feb 28, 2025
@noliveleger noliveleger removed the request for review from jnm February 28, 2025 09:36
@noliveleger noliveleger changed the base branch from main to release/2.024.36 February 28, 2025 09:41
@noliveleger noliveleger changed the title fix(trashBin): Ensure the project is fully restored from the trash fix(trashBin): ensure the project is fully restored from the trash TASK-1645 Feb 28, 2025
Comment on lines 129 to 144
asset = Asset.all_objects.only(
'pk', 'name', 'uid', 'owner_id'
).get(uid=self.kpi_asset_uid)
except Asset.DoesNotExist:
try:
asset = Asset.objects.only('pk').get(
_deployment_data__formid=self.pk
)
asset = Asset.all_objects.only(
'pk', 'name', 'uid', 'owner_id'
).get(_deployment_data__formid=self.pk)
except Asset.DoesNotExist:
# An `Asset` object needs to be returned to avoid 500 while
# Enketo is fetching for project XML (e.g: /formList, /manifest)
asset = Asset(uid=self.id_string)
asset = Asset(
uid=self.id_string,
name=self.title,
owner_id=self.user.id,
)
Copy link
Contributor Author

@noliveleger noliveleger Feb 28, 2025

Choose a reason for hiding this comment

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

Part of #5514 , add new fields to avoids conflicts with release/2.025.02

@noliveleger noliveleger changed the title fix(trashBin): ensure the project is fully restored from the trash TASK-1645 fix(trashBin): ensure the project is fully restored from the admin interface TASK-1645 Feb 28, 2025
@noliveleger noliveleger requested a review from rgraber February 28, 2025 16:36
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.

2 small things

@@ -126,16 +126,22 @@ def asset(self):
# uses an Asset object only to narrow down a query with a filter,
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is no longer accurate

)
asset = Asset.all_objects.only(
'pk', 'name', 'uid', 'owner_id'
).get(_deployment_data__formid=self.pk)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think using only helps us any here if we have to call get on a different field anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does ;-)

In [1]: Asset.objects.only('pk', 'uid', 'name').get(_deployment_data__backend_response__formid=428)
SELECT "kpi_asset"."id",
       "kpi_asset"."name",
       "kpi_asset"."uid"
  FROM "kpi_asset"
 WHERE (NOT ("kpi_asset"."pending_delete") AND ("kpi_asset"."_deployment_data" #> '{backend_response,formid}') = '428'::jsonb)
 LIMIT 21

Execution time: 0.013126s [Database: default]
Out[1]: <Asset: Project with audio (as2gssYmeivHnk5f7zXPbY)>

Having that said, you made see there was a problem with the condition ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. Django is smarter than I gave it credit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants