Skip to content

Commit

Permalink
feat(projectHistoryLog): record submission modifications (#5404)
Browse files Browse the repository at this point in the history
### 📣 Summary
Log updates to submissions, both content and validation status.

### 💭 Notes

#### SubmissionUpdate

The `SubmissionUpdate` class is just a dataclass meant to make it clear
what information we need to create a PH log for submission
modification/creation/deletion. It's meant mostly to help future
developers since we're not using the usual `initial_data`/`updated_data`
pattern where we just take configured fields off of a model.

#### ProjectHistoryLog.__init__
 
This PR includes a small refactor on ProjectHistoryLog to set the
`app_name`, `model_name`, and `log_type` in the `__init__` method like
we do with `create`. This saves us some repeated code when we are
creating logs in bulk.

#### Attaching the asset to the request

We always need the `asset_uid` when logging any views that include the
`AssetNestedObjectViewsetMixin` mixin, so I updated `AuditLoggedViewSet`
to always attach the asset (available via the mixin) to the request. We
may be able to use this to simplify other code later if we wish.

#### Where we add data to the request

We can't use the usual method of having the views inherit from
`AuditLoggedViewSet` and configuring `logged_fields` on the relevant
model because the viewsets that update Instances rarely act directly on
the Instance objects via get_object or the serializers. Instead they
usually have custom code that works through the deployment object on the
asset.

We could pass the request from the viewset through the layers of other
methods until we get to the part where we are actually updating
instances, but this would require a lot of changes in existing view code
and we're trying to keep the audit log code relatively self-contained.
Changes to logging code should ideally not require changes in view code.
Instead, I decided to use a receiver on the Instance model `post_save`
signal.

This also should allow the code to be more backend-agnostic since we did
not update anything directly in the OpenRosaDeploymentBackend model,
where most of the actual updating takes place. Theoretically, any
backend would have to eventually update the Instance model with the new
`xml` and `status` fields, regardless of how else it went about
performing the updates. This is an assumption we have to make since we
don't have any other "real" backend implementations, but it seems a safe
one.

The exception to this is when updating multiple validations statuses,
because that is done with a bulk update, which doesn't send the
`post_save` signal. In that bit of code, we have to manually attach the
updated instances to the request. It's not considered great practice to
get the request as part of model code but the alternative once again
requires a significant refactor to pass the request through multiple
layers.



### 👀 Preview steps

Feature/no-change template:
1. Regression test: follow the test plan at
#5387 to make sure that logging
for duplicate submissions still works since that changed.
1. ℹ️ have at least 2 accounts and a project. Make sure both accounts
have permission to add submissions to the project. For this preview,
assume user1 owns the project.
2. Add a submission as the owner
3. Log in as user2 and add a submission. You may want to do this in a
separate private tab, otherwise sometimes the enketo login stays cached
and the submissions end up attached to the first user.
4. Login as user1 again and enable submissions without username or email
5. Add one more submission. It should come in as anonymous (ie no
_submitted_by)
6. Go to the My Project > Data.
7. Change the validation status of a user1's submission to 'On Hold'
using the dropdown in the table.
8. Go to `/api/v2/assets/<asset_uid>/history`
9. 🟢 There should be a new project history log with
`action="modify-submission"` and the usual metadata, plus
```
"submission": {
    "status": "On Hold",
    "submitted_by": "user1"
}
```
10. Go back to the table and select all 3 submissions.
11. Select 'Change status' and update the status to 'Approved' for all
of them
12. 🟢 Reload the endpoint. There should be 3 new logs with
`action="modify-submission"` and the usual metadata, plus
```
"submission": {
    "status": "Approved",
    "submitted_by": "user1/user2/AnonymousUser"
}
```
Note: there should be one log each for the different users since they
each have one submission
13. Go back to the table and click the pencil by user2's submission.
Edit an answer to a question.
14. 🟢 Reload the endpoint. There should be a new log with
`action="modify-submission"` and the usual metadata, plus
```
"submission": {
    "submitted_by": "user2"
}
```
Note there is no validation status since that did not change.
15. Go back to the table and select all submissions.
16. Hit `Edit` and edit the answer to a question
17. 🟢 Reload the endpoint. There should be a 3 new logs with
`action="modify-submission"` and the usual metadata, plus
```
"submission": {
    "submitted_by": "user1/user2/AnonymousUser"
}
```
  • Loading branch information
rgraber authored Jan 9, 2025
1 parent 0666f7b commit 36b4484
Show file tree
Hide file tree
Showing 11 changed files with 380 additions and 59 deletions.
2 changes: 2 additions & 0 deletions kobo/apps/audit_log/audit_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class AuditAction(models.TextChoices):
DELETE = 'delete'
DELETE_MEDIA = 'delete-media'
DELETE_SERVICE = 'delete-service'
DELETE_SUBMISSION = 'delete-submission'
DEPLOY = 'deploy'
DISABLE_SHARING = 'disable-sharing'
DISALLOW_ANONYMOUS_SUBMISSIONS = 'disallow-anonymous-submissions'
Expand All @@ -23,6 +24,7 @@ class AuditAction(models.TextChoices):
MODIFY_IMPORTED_FIELDS = 'modify-imported-fields'
MODIFY_SERVICE = 'modify-service'
MODIFY_SHARING = 'modify-sharing'
MODIFY_SUBMISSION = 'modify-submission'
MODIFY_USER_PERMISSIONS = 'modify-user-permissions'
PUT_BACK = 'put-back'
REDEPLOY = 'redeploy'
Expand Down
4 changes: 4 additions & 0 deletions kobo/apps/audit_log/base_views.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from rest_framework import mixins, viewsets

from kpi.utils.viewset_mixins import AssetNestedObjectViewsetMixin


def get_nested_field(obj, field: str):
"""
Expand Down Expand Up @@ -40,6 +42,8 @@ def initialize_request(self, request, *args, **kwargs):
request = super().initialize_request(request, *args, **kwargs)
request._request.log_type = self.log_type
request._request._data = request.data.copy()
if isinstance(self, AssetNestedObjectViewsetMixin):
request._request.asset = self.asset
return request

def get_object(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Generated by Django 4.2.15 on 2025-01-01 17:33

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('audit_log', '0014_add_submission_action'),
]

operations = [
migrations.AlterField(
model_name='auditlog',
name='action',
field=models.CharField(
choices=[
('add-media', 'Add Media'),
('add-submission', 'Add Submission'),
('allow-anonymous-submissions', 'Allow Anonymous Submissions'),
('archive', 'Archive'),
('auth', 'Auth'),
('clone-permissions', 'Clone Permissions'),
('connect-project', 'Connect Project'),
('create', 'Create'),
('delete', 'Delete'),
('delete-media', 'Delete Media'),
('delete-service', 'Delete Service'),
('delete-submission', 'Delete Submission'),
('deploy', 'Deploy'),
('disable-sharing', 'Disable Sharing'),
(
'disallow-anonymous-submissions',
'Disallow Anonymous Submissions',
),
('disconnect-project', 'Disconnect Project'),
('enable-sharing', 'Enable Sharing'),
('export', 'Export'),
('in-trash', 'In Trash'),
('modify-imported-fields', 'Modify Imported Fields'),
('modify-service', 'Modify Service'),
('modify-sharing', 'Modify Sharing'),
('modify-submission', 'Modify Submission'),
('modify-user-permissions', 'Modify User Permissions'),
('put-back', 'Put Back'),
('redeploy', 'Redeploy'),
('register-service', 'Register Service'),
('remove', 'Remove'),
('replace-form', 'Replace Form'),
('share-form-publicly', 'Share Form Publicly'),
('share-data-publicly', 'Share Data Publicly'),
('unarchive', 'Unarchive'),
('unshare-form-publicly', 'Unshare Form Publicly'),
('unshare-data-publicly', 'Unshare Data Publicly'),
('update', 'Update'),
('update-content', 'Update Content'),
('update-name', 'Update Name'),
('update-settings', 'Update Settings'),
('update-qa', 'Update Qa'),
('transfer', 'Transfer'),
],
db_index=True,
default='delete',
max_length=30,
),
),
]
76 changes: 49 additions & 27 deletions kobo/apps/audit_log/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from kobo.apps.audit_log.audit_log_metadata_schemas import (
PROJECT_HISTORY_LOG_METADATA_SCHEMA,
)
from kobo.apps.audit_log.utils import SubmissionUpdate
from kobo.apps.kobo_auth.shortcuts import User
from kobo.apps.openrosa.libs.utils.viewer_tools import (
get_client_ip,
Expand Down Expand Up @@ -293,6 +294,7 @@ def create_from_request(


class ProjectHistoryLogManager(models.Manager, IgnoreCommonFieldsMixin):

def get_queryset(self):
return super().get_queryset().filter(log_type=AuditType.PROJECT_HISTORY)

Expand Down Expand Up @@ -323,6 +325,12 @@ class ProjectHistoryLog(AuditLog):
class Meta:
proxy = True

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.app_label = Asset._meta.app_label
self.model_name = Asset._meta.model_name
self.log_type = AuditType.PROJECT_HISTORY

@classmethod
def create_from_import_task(cls, task: ImportTask):
# this will probably only ever be a list of size 1 or 0,
Expand Down Expand Up @@ -383,7 +391,11 @@ def create_from_request(cls, request: WSGIRequest):
'asset-permission-assignment-list': cls._create_from_permissions_request,
'asset-permission-assignment-clone': cls._create_from_clone_permission_request, # noqa
'project-ownership-invite-list': cls._create_from_ownership_transfer,
'submission-duplicate': cls._create_from_duplicate_request,
'submission-duplicate': cls._create_from_submission_request,
'submission-bulk': cls._create_from_submission_request,
'submission-validation-statuses': cls._create_from_submission_request,
'submission-validation-status': cls._create_from_submission_request,
'assetsnapshot-submission-alias': cls._create_from_submission_request,
}
url_name = request.resolver_match.url_name
method = url_name_to_action.get(url_name, None)
Expand Down Expand Up @@ -568,26 +580,6 @@ def _create_from_detail_request(cls, request):
metadata=full_metadata,
)

@classmethod
def _create_from_duplicate_request(cls, request):
asset_uid = request.resolver_match.kwargs['parent_lookup_asset']
updated_data = request.updated_data
metadata = {
'ip_address': get_client_ip(request),
'source': get_human_readable_client_user_agent(request),
'asset_uid': asset_uid,
'log_subtype': PROJECT_HISTORY_LOG_PROJECT_SUBTYPE,
'submission': {
'submitted_by': updated_data['submitted_by']
}
}
ProjectHistoryLog.objects.create(
object_id=updated_data['asset_id'],
action=AuditAction.ADD_SUBMISSION,
user=request.user,
metadata=metadata
)

@classmethod
def _create_from_export_request(cls, request):
cls._related_request_base(request, None, AuditAction.EXPORT, None, None)
Expand All @@ -609,6 +601,42 @@ def _create_from_hook_request(cls, request):
AuditAction.MODIFY_SERVICE,
)

@classmethod
def _create_from_submission_request(cls, request):
if request.method in ['GET', 'HEAD']:
return
instances: dict[int:SubmissionUpdate] = getattr(request, 'instances', {})
logs = []
url_name = request.resolver_match.url_name

for instance in instances.values():
if instance.action == 'add':
action = AuditAction.ADD_SUBMISSION
else:
action = AuditAction.MODIFY_SUBMISSION
metadata = {
'asset_uid': request.asset.uid,
'log_subtype': PROJECT_HISTORY_LOG_PROJECT_SUBTYPE,
'ip_address': get_client_ip(request),
'source': get_human_readable_client_user_agent(request),
'submission': {
'submitted_by': instance.username,
},
}
if 'validation-status' in url_name:
metadata['submission']['status'] = instance.status

logs.append(
ProjectHistoryLog(
user=request.user,
object_id=request.asset.id,
action=action,
user_uid=request.user.extra_details.uid,
metadata=metadata,
)
)
ProjectHistoryLog.objects.bulk_create(logs)

@classmethod
def _create_from_ownership_transfer(cls, request):
updated_data = getattr(request, 'updated_data')
Expand All @@ -624,9 +652,6 @@ def _create_from_ownership_transfer(cls, request):
object_id=transfer['asset__id'],
action=AuditAction.TRANSFER,
user=request.user,
app_label=Asset._meta.app_label,
model_name=Asset._meta.model_name,
log_type=AuditType.PROJECT_HISTORY,
user_uid=request.user.extra_details.uid,
metadata={
'asset_uid': transfer['asset__uid'],
Expand Down Expand Up @@ -676,9 +701,6 @@ def _create_from_permissions_request(cls, request):
log_base = {
'user': request.user,
'object_id': asset_id,
'app_label': Asset._meta.app_label,
'model_name': Asset._meta.model_name,
'log_type': AuditType.PROJECT_HISTORY,
'user_uid': request.user.extra_details.uid,
}
# get all users whose permissions changed
Expand Down
23 changes: 23 additions & 0 deletions kobo/apps/audit_log/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from celery.signals import task_success
from django.contrib.auth.signals import user_logged_in
from django.db.models.signals import post_save
from django.dispatch import receiver
from django_userforeignkey.request import get_current_request

Expand All @@ -15,7 +16,9 @@
post_remove_partial_perms,
post_remove_perm,
)
from ..openrosa.apps.logger.models import Instance
from .models import AccessLog, ProjectHistoryLog
from .utils import SubmissionUpdate

# Access Log receivers

Expand Down Expand Up @@ -69,6 +72,26 @@ def add_assigned_partial_perms(sender, instance, user, perms, **kwargs):
request.partial_permissions_added[user.username] = perms_as_list_of_dicts


@receiver(post_save, sender=Instance)
def add_instance_to_request(instance, created, **kwargs):
request = get_current_request()
if request is None:
return
if getattr(request, 'instances', None) is None:
request.instances = {}
username = instance.user.username if instance.user else None
request.instances.update(
{
instance.id: SubmissionUpdate(
username=username,
status=instance.get_validation_status().get('label', 'None'),
action='add' if created else 'modify',
id=instance.id,
)
}
)


@receiver(post_remove_perm, sender=Asset)
def add_removed_perms(sender, instance, user, codename, **kwargs):
request = _initialize_request()
Expand Down
Loading

0 comments on commit 36b4484

Please sign in to comment.