Skip to content

Conversation

@dmartin4820
Copy link
Member

@dmartin4820 dmartin4820 commented Sep 25, 2025

Fixes #37

What changes did you make?

  • create permission_history model
  • register permission_history in admin
  • add permission_history endpoints
  • add permission_history seed data
  • add api and model tests for permission_history creation

@dmartin4820 dmartin4820 requested a review from del9ra September 25, 2025 05:20
@dmartin4820 dmartin4820 force-pushed the create_table_permission_history_37 branch from 50d1cab to 8b672ca Compare September 25, 2025 05:34
@fyliu fyliu moved this to PR Needs review (automated column, do not place items here manually) in P: PD: Project Board Oct 3, 2025
Copy link
Member

@del9ra del9ra left a comment

Choose a reason for hiding this comment

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

Code looks mostly good! I just have one question: why did you use CASCADE instead of PROTECT for the history table? Wouldn't we lose data if a user is deleted?

created_by and updated_by should be integers according to the issue.

@github-project-automation github-project-automation bot moved this from PR Needs review (automated column, do not place items here manually) to PR changes requested in P: PD: Project Board Oct 17, 2025
@dmartin4820
Copy link
Member Author

dmartin4820 commented Oct 23, 2025

Code looks mostly good! I just have one question: why did you use CASCADE instead of PROTECT for the history table? Wouldn't we lose data if a user is deleted?

created_by and updated_by should be integers according to the issue.

@del9ra The CASCADE instead of PROTECT can be fixed easily. It should be PROTECT and I originally misunderstood how it worked at first but it's clearer now.

For created_by and updated_by, I stuck with foreign keys based on @fyliu 's comment but there isn't really a confirmation from others on whether there is an explicit reason for it being an integer. The best reason I found was that making it a foreign key provides more functionality that I might be currently unaware of. One thing being that it allows us to specify on_delete if made a foreign key using Django. With an integer those same protections aren't there.

I can submit a change once resolved

@dmartin4820 dmartin4820 force-pushed the create_table_permission_history_37 branch from 8b672ca to 3144807 Compare October 24, 2025 03:20
@dmartin4820 dmartin4820 force-pushed the create_table_permission_history_37 branch from 3144807 to c5a5cea Compare October 24, 2025 03:33
@dmartin4820 dmartin4820 requested a review from del9ra October 24, 2025 03:33
@ExperimentsInHonesty
Copy link
Member

Code looks mostly good! I just have one question: why did you use CASCADE instead of PROTECT for the history table? Wouldn't we lose data if a user is deleted?
created_by and updated_by should be integers according to the issue.

@del9ra The CASCADE instead of PROTECT can be fixed easily. It should be PROTECT and I originally misunderstood how it worked at first but it's clearer now.

For created_by and updated_by, I stuck with foreign keys based on @fyliu 's comment but there isn't really a confirmation from others on whether there is an explicit reason for it being an integer. The best reason I found was that making it a foreign key provides more functionality that I might be currently unaware of. One thing being that it allows us to specify on_delete if made a foreign key using Django. With an integer those same protections aren't there.

I can submit a change once resolved

We talked about this during the 2025-11-20 team meeting and the created_by and updated_by are FKs to the user table. The issue has been updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: PR changes requested

Development

Successfully merging this pull request may close these issues.

Create Table: permission_history

3 participants