lix-community/nix-security-tracker

Fork 0

Suggestions activity log #366

Merged

alejandrosame merged 3 commits from feat/activity-log-suggestions into main

2024-11-28 10:01:31 +00:00

alejandrosame commented

2024-11-17 05:29:21 +00:00

(Migrated from github.com)

The activity log is implemented with the package 'django-pghistory', which uses PostgreSQL triggers.

- Added python package 'django-pghistory'.
- Updated 'django-pgtrigger' version, a dependency of the former. The old
  version was conflicting.

The activity log is implemented with the package 'django-pghistory', which uses PostgreSQL triggers. - Added python package 'django-pghistory'. - Updated 'django-pgtrigger' version, a dependency of the former. The old version was conflicting.

RaitoBezarius (Migrated from github.com) reviewed

2024-11-17 19:21:12 +00:00

src/website/webview/views.py

RaitoBezarius (Migrated from github.com) commented

2024-11-17 19:21:11 +00:00

.all().iterator() should be preferred for iteration

`.all().iterator()` should be preferred for iteration

RaitoBezarius (Migrated from github.com) reviewed

2024-11-17 19:24:06 +00:00

src/website/webview/views.py

RaitoBezarius (Migrated from github.com) commented

2024-11-17 19:24:06 +00:00

the business logic of this should be moved to SQL I think, this may require a more complicated SQL query but it's fine

RaitoBezarius (Migrated from github.com) reviewed

2024-11-17 19:25:49 +00:00

src/website/webview/views.py

RaitoBezarius (Migrated from github.com) commented

2024-11-17 19:25:49 +00:00

this is inefficient, you should have get_suggestion_activity_logs(<list of object>) and returns a map of {pk: logs} and then interleave them there.

this is inefficient, you should have `get_suggestion_activity_logs(<list of object>)` and returns a map of `{pk: logs}` and then interleave them there.

alejandrosame (Migrated from github.com) reviewed

2024-11-18 00:37:31 +00:00

src/website/webview/views.py

alejandrosame (Migrated from github.com) commented

2024-11-18 00:37:31 +00:00

For clarification, my guess is that you are referring to making it part of the ORM expression and not a RAW SQL call, right?

alejandrosame commented

2024-11-18 00:40:12 +00:00

(Migrated from github.com)

@RaitoBezarius I'm still massaging the data for the view. I'll take into account these query feedback on the last refactors before taking this PR out of draft.

fricklerhandwerk commented

2024-11-20 09:01:44 +00:00

(Migrated from github.com)

Tested, looking good! 🎉

A few things currently on my mind:

We wanted to show relative times first, so those still needs a conversion
We only need to show only the creation timestamp in the suggestions view (there can't be any activity anyway at that point)
Showing the creation timestamp should always be possible (even before the logs were enabled), it's recorded with the suggestion in the TimeStampMixin
The logs take 750ms per item on my machine
- @RaitoBezarius says there could be some quick wins, so now it's time to harvest those
- Eventually we probably want to not load the logs for the entire list, but only on demand
  - It should be enough to show last-modified in the <summary>
  - Last-modified can also be recorded in the TimeStampMixin, but the needs to be overridden to keep only the state changes, because IIUC it's not doing anything right now for suggestions

Tested, looking good! :tada: A few things currently on my mind: - We wanted to show relative times first, so those still needs a conversion - We only need to show only the creation timestamp in the suggestions view (there can't be any activity anyway at that point) - Showing the creation timestamp should always be possible (even before the logs were enabled), it's recorded with the suggestion in the `TimeStampMixin` - The logs take 750ms per item on my machine - @RaitoBezarius says there could be some quick wins, so now it's time to harvest those - Eventually we probably want to not load the logs for the entire list, but only on demand - It should be enough to show last-modified in the `<summary>` - Last-modified can also be recorded in the `TimeStampMixin`, but the needs to be overridden to keep only the state changes, because IIUC it's not doing anything right now for suggestions

👍 1

erictapen commented

2024-11-20 14:08:21 +00:00

(Migrated from github.com)

Happy to be able to look at it now! I'd keep design feedback until it's marked as ready for review.
Just one hint: With https://github.com/Nix-Security-WG/nix-security-tracker/pull/374 a color palette was introduced, that could be used for the coloring of state change activities.

Happy to be able to look at it now! I'd keep design feedback until it's marked as ready for review. Just one hint: With https://github.com/Nix-Security-WG/nix-security-tracker/pull/374 a color palette was introduced, that could be used for the coloring of state change activities.

👍 1

alejandrosame (Migrated from github.com) reviewed

2024-11-21 16:04:19 +00:00

src/website/webview/views.py

alejandrosame (Migrated from github.com) commented

2024-11-21 16:04:19 +00:00

From https://github.com/Nix-Security-WG/nix-security-tracker/pull/366#issuecomment-2487949573

Eventually we probably want to not load the logs for the entire list, but only on demand

@fricklerhandwerk Is this the case? Because in that case I'd skip this optimization, since it's the current logic is already tailored to "gather the activity log of this suggestion".

From https://github.com/Nix-Security-WG/nix-security-tracker/pull/366#issuecomment-2487949573 >Eventually we probably want to not load the logs for the entire list, but only on demand @fricklerhandwerk Is this the case? Because in that case I'd skip this optimization, since it's the current logic is already tailored to "gather the activity log of this suggestion".