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
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

2 participants