feat(accessLogExport)!: create new AccessLogExportTask to generate a csv of access logs TASK-871 #5258

RuthShryock · 2024-11-12T22:10:58Z

👀 Preview steps

Ensure that the Export all data button for ProjectViews still works as normal and that the email header has the appropriate title: "Project View Report Complete"
Ensure that exports for submission data works correctly:

make submissions to a project
navigate to the Project < Data < Downloads
🟢 ensure the Export button is exporting data correctly

Preview the AccessLogExportTask from the shell:

ℹ️ have a user
enter the Django shell: ./manage.py shell_plus
get your user: test_user = User.objects.get(username='test')
create an AccessLogExportTask:

task = AccessLogExportTask.objects.create(
           user=test_user,
           get_all_logs=False,
           data={'type': 'access_logs_export'},
  )

🟢 print the task object and notice that an AccessLogExportTask object has been created: print(task)
run the task: task.run()
🟢 print the result and notice that it generated a link to the csv export: print(task.result)

💭 Notes

Changed the field named user to user_url because naming it user conflicts with an existing field in the AccessLog model
Renamed the file titled project_view_exports.py to data_exports.py because it now generates exports for both project view and access log exports
Refactored the ProjectViewExportTask to have a base CommonExportTask which both the project view and access log export classes inherit from
Updated the email (in kpi/tasks.py) to have the correct subject title based on the type of export report it is sending
Alphabetized the functions in data_exports.py

…ate csv files for access logs, and added tests

notion-workspace · 2024-11-12T22:11:05Z

Create AccessLogExportTask class

jnm

A nitpick that I need to somehow get into our organization-wide development guidelines: SubmissionsExport… should be SubmissionExport. Another example of something similar would be that we use AssetSerializer instead of AssetsSerializer

…ort-task-class

jamesrkiger

We should figure out what is going on with the diff here (208 files changed) before we proceed with this. I'm sure whoever else looks at this will notice, but I'm marking it as change requested out of an abundance of caution.

RuthShryock · 2024-11-14T22:17:10Z

@jamesrkiger Yes sorry I'm working on cleaning this up, something with my force push went wrong.

…ission prefix

…ort-task-class

…view task

rgraber

I think we can simplify this refactor a bit.
If we make CommonExportTask a mixin we won't need to do the difficult migration.
Also, I think instead of having a lot of methods that are like

do_thing(task):
    if task is project_view:
        do_stuff
    elif task is export:
        do other stuff

we can make the task classes themselves responsible for generating the data.
Then export_task_in_background would look like (pseudocode)

model = get_model()
task = model.get(uid=uid)
task.run()
if status is complete:
      subject = model.default_email_subject
       ...get the result and send the email

and in CommonExportTask, run_task could do something like

data = self.get_data()
url = create_export_file()
return url

and AccessLogExportTask and ProjectViewExportTask would each have their own implementations of get_data() and a value for default_email_subject.
Usually inheritance is preferable to if-else chains because the code it's easier to change one subclass without changing methods that affect other subclasses.

rgraber · 2024-11-20T19:37:15Z

kpi/models/import_export_task.py

@@ -482,27 +482,23 @@ def export_upload_to(self, filename):
    return posixpath.join(self.user.username, 'exports', filename)


-class ProjectViewExportTask(ImportExportTask):
-    uid = KpiUidField(uid_prefix='pve')
+class CommonExportTask(ImportExportTask):


I should have caught this before, but I think we may be able to avoid the complicated migration if we make this a mixin rather than a separate model.

rgraber · 2024-11-20T20:10:00Z

kpi/utils/data_exports.py

 }


+def create_data_export(


If we do it this way we'll have to add to this method every time we create a new kind of export task. I think it would make more sense to have this method take just take data and headers and create a csv from it. The AccessLogExportTask and ProjectViewExportTask classes would do whatever they had to do to get the data and then call this method with the data they collected.

rgraber · 2024-11-20T20:13:22Z

kpi/utils/data_exports.py

-    if isinstance(val, str):
-        val = val.replace('\n', '')
-    return val
+def get_data(filtered_queryset: QuerySet, export_type: str) -> QuerySet:


now that we have more cases, we should just have each type of ExportTask have its own method for getting data so we don't just keep adding elifs for every new kind of export task.

rgraber · 2024-11-20T20:32:51Z

kpi/migrations/0060_commonexporttask_remove_projectviewexporttask_data_and_more.py

+                'abstract': False,
+            },
+        ),
+        migrations.RunPython(populate_common_export_tasks),


Forgot: if we end up keeping the migration, this needs to be reversible, even if the reverse is RunPython.noop. Otherwise unapplying migrations breaks

…t_email_subject to the ProjectView and AccessLog export tasks

rgraber

some small things

rgraber · 2024-11-27T13:21:23Z

kobo/apps/audit_log/models.py

@@ -610,7 +610,7 @@ def create_from_related_request(
            action = modify_action
        if action:
            # some actions on related objects do not need to be logged,
-            # eg deleting an ExportTask
+            # eg deleting an SubmissionExportTask


Suggested change

# eg deleting an SubmissionExportTask

# eg deleting a SubmissionExportTask

nit, definitely non-blocking

rgraber · 2024-11-27T13:44:15Z

kpi/models/import_export_task.py

-        filename = self._build_export_filename(
-            export_type, self.user.username, view
-        )
+    def _run_task_base(self, messages: list, buff) -> None:


Suggested change

def _run_task_base(self, messages: list, buff) -> None:

def _export_data_to_file(self, messages: list, buff) -> None:

since it's no longer doing the whole task

rgraber · 2024-11-27T13:44:53Z

kpi/models/import_export_task.py

+        return 'Access Log Report Complete'
+
+    def get_data(self, filtered_queryset: QuerySet) -> QuerySet:
+        user_url = Concat(


didn't know you could do this outside a query. neat.

rgraber · 2024-11-27T15:33:04Z

kpi/models/import_export_task.py

+        return 'Report Complete'
+
+    def _get_export_details(self) -> tuple:
+        return self.data['type'], self.data['view']


This should account for data not having a view key since we don't need one for access logs exports. the filename can just be export_type-username-time for tasks without a view.

rgraber · 2024-11-27T15:33:32Z

kpi/models/import_export_task.py

+        return 'Report Complete'
+
+    def _get_export_details(self) -> tuple:
+        return self.data['type'], self.data['view']

    def _build_export_filename(
        self, export_type: str, username: str, view: str
    ) -> str:
        time = timezone.now().strftime('%Y-%m-%dT%H:%M:%SZ')
        return f'{export_type}-{username}-view_{view}-{time}.csv'


Same as previous comment. This should be updated to allow an empty/None view parameter

rgraber · 2024-11-27T15:41:15Z

kpi/models/import_export_task.py

+            'ip_address',
+            'initial_user_username',
+            'initial_user_uid',
+            'auth_app_name',


Suggested change

'auth_app_name',

'authorized_app_name',

…e None

rgraber

Just the ordering thing and then I think we're good to go!

rgraber · 2024-12-02T13:22:13Z

kpi/tests/test_access_logs_export_task.py

+from kobo.apps.audit_log.models import AccessLog
+from kpi.models.import_export_task import AccessLogExportTask
+
+User = get_user_model()


nit (non-blocking)
If you want to avoid this boilerplate, you can use from kobo.apps.kobo_auth.shortcuts import User

rgraber · 2024-12-02T13:24:01Z

kpi/tests/test_access_logs_export_task.py

+
+        self.assertIsNotNone(task.result, 'The task.result should not be None.')
+        expected_pattern = (
+            rf'{self.superuser.username}/exports/access_logs_export-'


you are way more patient than I am to write this regex. respect.

rgraber · 2024-12-02T13:53:39Z

kpi/utils/data_exports.py

@@ -79,9 +92,43 @@
        'key': METADATA,
        'columns': USER_FIELDS + METADATA_FIELDS,
    },
+    'access_logs_export': {


We should order by '-date-created to be consistent with the endpoint

…ort-task-class

rgraber

LGTM

add AccessLogExportTask class, refactored Project View exports to cre…

09373fb

…ate csv files for access logs, and added tests

RuthShryock added the Back end label Nov 12, 2024

RuthShryock requested a review from rgraber November 12, 2024 22:10

RuthShryock changed the title ~~feat(accessLogExportTask): create new AccessLogExportTask to generate a csv of access logs TASK-871~~ feat(accessLogExport): create new AccessLogExportTask to generate a csv of access logs TASK-871 Nov 12, 2024

RuthShryock self-assigned this Nov 12, 2024

RuthShryock marked this pull request as ready for review November 13, 2024 16:55

RuthShryock requested review from jnm, noliveleger and magicznyleszek as code owners November 13, 2024 16:55

magicznyleszek removed their request for review November 14, 2024 08:54

RuthShryock changed the title ~~feat(accessLogExport): create new AccessLogExportTask to generate a csv of access logs TASK-871~~ feat(accessLogExport)!: create new AccessLogExportTask to generate a csv of access logs TASK-871 Nov 14, 2024

jnm requested changes Nov 14, 2024

View reviewed changes

revert to before Submissions name change

4079ea0

RuthShryock force-pushed the access-log-export-task-class branch from eadbd10 to 4079ea0 Compare November 14, 2024 21:22

RuthShryock requested review from Akuukis, Guitlle and jamesrkiger as code owners November 14, 2024 21:22

RuthShryock removed request for Akuukis, Guitlle and jamesrkiger November 14, 2024 21:22

RuthShryock added 2 commits November 14, 2024 16:47

:This reverts commit 4079ea0.

eb67d2b

Merge branch 'main' of github.com:kobotoolbox/kpi into access-log-exp…

0f9ac39

…ort-task-class

jamesrkiger requested changes Nov 14, 2024

View reviewed changes

fix circular import error

2836ea3

RuthShryock added 4 commits November 14, 2024 17:28

fix formatting

b69a006

rename ExportTaskBase, ExportTask, and SynchronousExport to have Subm…

04dc2cd

…ission prefix

fix formatting

4c46fb0

Merge branch 'main' of github.com:kobotoolbox/kpi into access-log-exp…

de7f7f5

…ort-task-class

RuthShryock added 2 commits November 20, 2024 09:26

fix migration error by creating common export tasks for each project …

ff45ba8

…view task

fix formatting

7837192

rgraber requested changes Nov 20, 2024

View reviewed changes

rgraber reviewed Nov 20, 2024

View reviewed changes

RuthShryock added 2 commits November 26, 2024 15:33

simplify refactor with ExportTaskMixin and adding get_data and defaul…

6c60768

…t_email_subject to the ProjectView and AccessLog export tasks

updating branch with main

43ce879

rgraber requested changes Nov 27, 2024

View reviewed changes

rename functions and variables, fix grammar, and allow for views to b…

bd5c994

…e None

rgraber requested changes Dec 2, 2024

View reviewed changes

RuthShryock added 3 commits December 2, 2024 11:22

order access logs by date_created and fix User imports in test

6eb71c2

Merge branch 'main' of github.com:kobotoolbox/kpi into access-log-exp…

a807f66

…ort-task-class

order exports in reverse order

83bf0ed

rgraber approved these changes Dec 2, 2024

View reviewed changes

RuthShryock merged commit 6992e8a into main Dec 2, 2024
8 checks passed

RuthShryock deleted the access-log-export-task-class branch December 2, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(accessLogExport)!: create new AccessLogExportTask to generate a csv of access logs TASK-871 #5258

feat(accessLogExport)!: create new AccessLogExportTask to generate a csv of access logs TASK-871 #5258

RuthShryock commented Nov 12, 2024 •

edited

Loading

notion-workspace bot commented Nov 12, 2024

jnm left a comment

jamesrkiger left a comment

RuthShryock commented Nov 14, 2024

rgraber left a comment

rgraber Nov 20, 2024

rgraber Nov 20, 2024

rgraber Nov 20, 2024

rgraber Nov 20, 2024

rgraber left a comment

rgraber Nov 27, 2024

rgraber Nov 27, 2024

rgraber Nov 27, 2024

rgraber Nov 27, 2024

rgraber Nov 27, 2024

rgraber Nov 27, 2024

rgraber left a comment

rgraber Dec 2, 2024

rgraber Dec 2, 2024

rgraber Dec 2, 2024

rgraber left a comment

	# eg deleting an SubmissionExportTask
	# eg deleting a SubmissionExportTask

	def _run_task_base(self, messages: list, buff) -> None:
	def _export_data_to_file(self, messages: list, buff) -> None:

feat(accessLogExport)!: create new AccessLogExportTask to generate a csv of access logs TASK-871 #5258

feat(accessLogExport)!: create new AccessLogExportTask to generate a csv of access logs TASK-871 #5258

Conversation

RuthShryock commented Nov 12, 2024 • edited Loading

👀 Preview steps

💭 Notes

notion-workspace bot commented Nov 12, 2024

jnm left a comment

Choose a reason for hiding this comment

jamesrkiger left a comment

Choose a reason for hiding this comment

RuthShryock commented Nov 14, 2024

rgraber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber left a comment

Choose a reason for hiding this comment

RuthShryock commented Nov 12, 2024 •

edited

Loading