lix-community/nix-security-tracker

Fork 0

feat: nixpkgsissue activity log view #126

Closed

alejandrosame wants to merge 2 commits from feat/activity-log-app into main

alejandrosame commented

2023-12-14 00:36:52 +00:00

(Migrated from github.com)

feat: nixpkgsissue activity log view

Improve the messy representation of the different pghistory event tables:

Create a view log_nixpkgsissue in the PostgreSQL backend and introduce an unmanaged model NixpkgsIssueLogView to display a list of changes per timestamp and user.
Register NixpkgsIssueLogView in the pghistory app to declutter the log reporting from the already crowded shared section.
Delete the previous activitiy log models and views in favour of the unified view model.

If performance starts being problematic for the log_nixpkgsissue view, we can
make it a materialized view and add a trigger to update its contents on shared.nixpkgsissue
and it's m2m relationships updates.

NOTE: This PR includes #101

feat: nixpkgsissue activity log view Improve the messy representation of the different `pghistory` event tables: - Create a view `log_nixpkgsissue` in the PostgreSQL backend and introduce an unmanaged model `NixpkgsIssueLogView` to display a list of changes per timestamp and user. - Register `NixpkgsIssueLogView` in the `pghistory` app to declutter the log reporting from the already crowded `shared` section. - Delete the previous activitiy log models and views in favour of the unified view model. If performance starts being problematic for the `log_nixpkgsissue` view, we can make it a materialized view and add a trigger to update its contents on `shared.nixpkgsissue` and it's m2m relationships updates. **NOTE:** This PR includes #101

RaitoBezarius (Migrated from github.com) reviewed

2023-12-14 14:26:53 +00:00

src/website/shared/models/cve.py

RaitoBezarius (Migrated from github.com) commented

2023-12-14 14:26:52 +00:00

# `cve` and `derivations` fields have to be tracked with via their `through` models

```suggestion # `cve` and `derivations` fields have to be tracked with via their `through` models ```

RaitoBezarius (Migrated from github.com) reviewed

2023-12-14 14:27:31 +00:00

src/website/shared/models/cve.py

RaitoBezarius (Migrated from github.com) commented

2023-12-14 14:27:31 +00:00

Can we add a comment on why this is not managed by the ORM?

RaitoBezarius (Migrated from github.com) approved these changes

2023-12-14 14:29:24 +00:00

RaitoBezarius (Migrated from github.com) left a comment

Looks mostly good to me, I am just thinking about the GDPR considerations with the activity log, it's a good idea to make the cascade do nothing in case of deletion, but I wonder if we ought to implement a soft delete operation and bake in GDPR stuff in there directly.

alejandrosame (Migrated from github.com) reviewed

2023-12-14 17:30:25 +00:00

src/website/shared/models/cve.py

alejandrosame (Migrated from github.com) commented

2023-12-14 17:30:25 +00:00

I pasted the wrong commit message in the PR description. I'll add similar comments in the model + the fact that Django doesn't seem to support multi table proxy models (which would be the equivalent of VIEW tables in ORM).

alejandrosame commented

2023-12-14 17:35:28 +00:00

(Migrated from github.com)

@RaitoBezarius the cascade does nothing because it prevents Django from trying to delete content from a postgres VIEW, which would just yield an error (it's read-only after all).

The data is collected from other auth and pghistory tables so we have to double check there what exactly we want to do on user deletion. There's also the default Django admin table, which registers less finegrained changes made by users.

I'll make an issue to not to forget about looking into this.

@RaitoBezarius the cascade does nothing because it prevents Django from trying to delete content from a postgres `VIEW`, which would just yield an error (it's read-only after all). The data is collected from other `auth` and `pghistory` tables so we have to double check there what exactly we want to do on user deletion. There's also the default Django `admin` table, which registers less finegrained changes made by users. I'll make an issue to not to forget about looking into this.

alejandrosame (Migrated from github.com) reviewed

2023-12-14 23:13:48 +00:00

src/website/shared/models/cve.py

alejandrosame (Migrated from github.com) commented

2023-12-14 23:13:48 +00:00

fixed

alejandrosame (Migrated from github.com) reviewed

2023-12-14 23:13:56 +00:00

src/website/shared/models/cve.py

alejandrosame (Migrated from github.com) commented

2023-12-14 23:13:56 +00:00

comment added

alejandrosame commented

2023-12-18 02:57:00 +00:00

(Migrated from github.com)

Please don't merge yet. Working on some other functionality I noticed a mistake on the way I set the migration file in this PR.

I'll take this out of draft as soon as I push the fix.

Please don't merge yet. Working on some other functionality I noticed a mistake on the way I set the migration file in this PR. I'll take this out of draft as soon as I push the fix.

alejandrosame commented

2023-12-19 20:34:38 +00:00

(Migrated from github.com)

The PR is in a mergeable state again.

alejandrosame commented

2023-12-20 18:23:44 +00:00

(Migrated from github.com)

rebased with latest changes

alejandrosame commented

2023-12-20 21:34:05 +00:00

(Migrated from github.com)

@RaitoBezarius if you can reconfirm that you are ok with this approach after the rebases, this is ready to be merged.

fricklerhandwerk commented

2024-09-23 10:35:11 +00:00

(Migrated from github.com)

Towards https://github.com/Nix-Security-WG/nix-security-tracker/issues/169

alejandrosame commented

2024-11-28 03:06:07 +00:00

(Migrated from github.com)

We have activity log for suggestions with #366. If we were to reintroduce activity logs for issues, it should be based on top of what was implemented there.