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

feat(projectHistoryLogs): project history logs for bulk actions TASK-1229 #5270

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Guitlle
Copy link
Contributor

@Guitlle Guitlle commented Nov 14, 2024

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

📖 Description

This PR adds support for bulk actions logging.

👀 Preview steps

  1. Create two new projects
  2. Deploy the projects
  3. Execute the bulk actions for the 2 projects (archive, unarchive, delete and undelete) (NOTE: the frontend currently doesn't allow me to do bulk archive/unarchive actions, only delete)
  4. To test the archive and unarchive, the UI may not allow you to perform these actions, however you can use curl:
$ curl -X POST -H "Authorization: Token xxx" -d "{\"payload\": {\"action\": \"unarchive\", \"asset_uids\": [\"ae6hQt9s2phfjRnsNphLdA\", \"auYiwftdP2g8axKkiUB8cE\"]}}" -H "Content-Type: application/json"  http://kf.kobo.local/api/v2/assets/bulk/ 

{"detail":"2 projects have been unarchived"}

$ curl -X POST -H "Authorization: Token xxx" -d "{\"payload\": {\"action\": \"archive\", \"asset_uids\": [\"ae6hQt9s2phfjRnsNphLdA\", \"auYiwftdP2g8axKkiUB8cE\"]}}" -H "Content-Type: application/json"  http://kf.kobo.local/api/v2/assets/bulk/ 

{"detail":"2 projects have been archived"}

$ curl -X POST -H "Authorization: Token xxx" -d "{\"payload\": {\"action\": \"undelete\", \"asset_uids\": [\"aUxHmB9wAqr5yVtZeW3sdc\", \"aqTWH6Wkv9wMbUL3PeLR44\"]}}" -H "Content-Type: application/json"  http://kf.kobo.local/api/v2/assets/bulk/

{"detail":"2 projects have been undeleted"}      
  1. Check the logs were created through the django shell
$ ./run.py -cf exec kpi bash 
$ ./manage.py shell_plus
...
In[]: us = User.objects.get(username="super_admin")
In[]:  for log in ProjectHistoryLog.objects.filter(user=us).order_by('-date_created')[0:8]: print(log.date_created, log.action, log.metadata['asset_uid'])

2024-11-14 23:50:19.482422+00:00 archive auYiwftdP2g8axKkiUB8cE
2024-11-14 23:50:19.468558+00:00 archive ae6hQt9s2phfjRnsNphLdA
2024-11-14 23:50:03.891367+00:00 unarchive auYiwftdP2g8axKkiUB8cE
2024-11-14 23:50:03.848762+00:00 unarchive ae6hQt9s2phfjRnsNphLdA
2024-11-14 18:36:23.187022+00:00 archive ae6hQt9s2phfjRnsNphLdA
2024-11-14 18:32:28.181161+00:00 delete aUxHmB9wAqr5yVtZeW3sdc
2024-11-14 18:32:27.498762+00:00 delete aqTWH6Wkv9wMbUL3PeLR44
...

💭 Notes

@Guitlle Guitlle marked this pull request as ready for review November 14, 2024 18:45
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.

Two things:

  1. It was super not-obvious on the task (sorry) but we only need to worry about bulk archiving and non-archiving.
  2. Can we stick with the pattern of adding things to the request, then going through ProjectHistoryLog.create_from_request? It is easier to reason about if the code all generally flows through one place

kobo/apps/audit_log/tests/test_project_history_logs.py Outdated Show resolved Hide resolved
@@ -1,7 +1,6 @@
# coding: utf-8
import time

import constance
Copy link
Contributor

Choose a reason for hiding this comment

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

why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a complain from the linter :S ... probably from some merge, that is not related to this PR

@@ -88,6 +89,11 @@ def __init__(
super().__init__(instance=instance, data=data, **kwargs)

def create(self, validated_data):
ProjectHistoryLog.create_from_bulk_action(
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't create the logs until after we've completed the actions. if something fails, we don't want a log saying it succeeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we put the logs creation in the middleware, it will run before the rest of the request processing. So I am wondering if we can detect if it's going to fail or maybe have a hook that runs after the successful response is created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I'm checking the middleware code and it should run after the action was executed.

@Guitlle Guitlle requested a review from rgraber November 19, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants