Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate org groups (hr_group -> admin_group, primary_group -> member_group) #1275

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

simensandhaug
Copy link
Contributor

@simensandhaug simensandhaug commented Nov 14, 2022

Changes

  • Change DB fields for hr_group and primary_group to admin_group and member_group for organizations
  • Change name and group type for all responsible groups
  • New ResponsibleGroup.name format "Organization:Type" instead of just "Type"
  • ResponsibleGroup.group_type format "Type" (e.g ADMIN or MEMBER)
  • Every ResponsibleGroup also has a pointer to a Group object that has outdated names
    • Previous : "Organization:(HR|Medlem):uuid"
    • New : "Organization:(Admin|Member):uuid"
  • These changes make it easier to navigate api.indokntnu.no/admin and the names are more descriptive
  • Change necessary instances of hr_group and primary_group

@vercel
Copy link

vercel bot commented Nov 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
indok-web ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 27, 2023 at 4:24PM (UTC)

@simensandhaug simensandhaug requested review from larwaa and hermannm and removed request for larwaa November 14, 2022 18:35
larwaa
larwaa previously requested changes Nov 14, 2022
Copy link
Member

@larwaa larwaa left a comment

Choose a reason for hiding this comment

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

Absolutely fantastic job with this PR. A migration such as this always introduces a lot of complexity, and I think you've handled it in an excellent manner! Have a look at the feedback on migrations which I think will make your life a bit easier, and the other comments that I hope you find useful 😊 Again, great work with this, you're really touching some of the more complex parts of the application 🏆

backend/apps/organizations/signals.py Outdated Show resolved Hide resolved
backend/apps/organizations/models.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #1275 (6a8a60e) into main (4d8fa0d) will decrease coverage by 0.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##             main    #1275      +/-   ##
==========================================
- Coverage   81.55%   81.54%   -0.01%     
==========================================
  Files          88       88              
  Lines        3166     3165       -1     
==========================================
- Hits         2582     2581       -1     
  Misses        584      584              
Flag Coverage Δ
apitests 81.54% <94.11%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
backend/apps/organizations/signals.py 79.41% <75.00%> (ø)
backend/apps/forms/signals.py 100.00% <100.00%> (ø)
backend/apps/organizations/models.py 85.00% <100.00%> (ø)
backend/apps/organizations/types.py 73.77% <100.00%> (ø)
backend/apps/permissions/constants.py 100.00% <100.00%> (ø)
backend/apps/permissions/signals.py 97.50% <100.00%> (-0.07%) ⬇️
backend/apps/events/resolvers.py 20.43% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@simensandhaug simensandhaug dismissed larwaa’s stale review December 3, 2022 22:10

fixed the requested changes

Copy link
Member

@larwaa larwaa left a comment

Choose a reason for hiding this comment

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

These are always a bit tricky, lots of moving parts, but I think it's more or less good to go, just a minor bug

backend/apps/organizations/signals.py Outdated Show resolved Hide resolved
backend/apps/organizations/signals.py Outdated Show resolved Hide resolved
simensandhaug and others added 2 commits February 20, 2023 18:31
remove print statement

Co-authored-by: Lars Waage <[email protected]>
remove comment

Co-authored-by: Lars Waage <[email protected]>
@simensandhaug
Copy link
Contributor Author

Requested changes should be fixed if i understood the problem with uuid correctly. If not im positive we can just remove the uuid from the name altogether, i cant see a reason why we would specifically need it in the name on the adminpage because it would be a field?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants