-
Notifications
You must be signed in to change notification settings - Fork 577
Fix #3417: Add data migration to create project_managers group #3948
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
mathjazz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thank you! 🥳
Note that adding Fixed #ISSUE_NUMBER links the PR to the said issue and fixes it automatically after the PR is merged. |
mathjazz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured there are a few ways to improve this.
Could you please check these suggestions?
| # 2. Get or Create the 'project_managers' group | ||
| group, created = Group.objects.get_or_create(name="project_managers") | ||
| if created: | ||
| print("Created 'project_managers' group.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use log.info() instead of print().
| # 3. Ensure the 'can_manage_project' permission exists | ||
| # We find the ContentType ID for the Project model | ||
| project_content_type = ContentType.objects.get_for_model(Project) | ||
|
|
||
| permission, perm_created = Permission.objects.get_or_create( | ||
| codename="can_manage_project", | ||
| content_type=project_content_type, | ||
| defaults={"name": "Can manage project"}, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ContentType.objects.get_for_model() in Django migrations is not inherently "safe", because it relies on the live application state rather than the historical migration state.
Also, we should not create the permission in the migration, since it's already created in the model.
| # 3. Ensure the 'can_manage_project' permission exists | |
| # We find the ContentType ID for the Project model | |
| project_content_type = ContentType.objects.get_for_model(Project) | |
| permission, perm_created = Permission.objects.get_or_create( | |
| codename="can_manage_project", | |
| content_type=project_content_type, | |
| defaults={"name": "Can manage project"}, | |
| ) | |
| # 3. Fetch the permission declared on Project.Meta.permissions | |
| project_content_type = ContentType.objects.get(app_label="base", model="project") | |
| permission = Permission.objects.get( | |
| codename="can_manage_project", | |
| content_type=project_content_type, | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the comment and permission definition as suggested above.
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(create_pm_group, remove_pm_group), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop remove_pm_group(), because it seems risky. It deletes the whole group, even if it existed before this migration.
| migrations.RunPython(create_pm_group, remove_pm_group), | |
| migrations.RunPython(create_pm_group, reverse_code=migrations.RunPython.noop), |
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Please check again suggested changes in the unresolved comment.
Please also make sure tests don't fail.
| # 3. Ensure the 'can_manage_project' permission exists | ||
| # We find the ContentType ID for the Project model | ||
| project_content_type = ContentType.objects.get_for_model(Project) | ||
|
|
||
| permission, perm_created = Permission.objects.get_or_create( | ||
| codename="can_manage_project", | ||
| content_type=project_content_type, | ||
| defaults={"name": "Can manage project"}, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the comment and permission definition as suggested above.
|
Hello @mathjazz I apologize for the delayed reply to the comments. I fixed the code with the suggestions you made, but I'm having a slight issue figuring out why I'm running into an error with the bot. I'm currently working on it now |
|
The error from the tests originates from the get() method. The method tries to fetch permissions that Django has not created yet. I added a line to trigger a population signal, but that crashed as well. After running 3 other tests and them failing due to missing tables in the migration, using get_or_create() will allow creating a specific contentType without triggering a registry scan and hitting a domino dependency issue. |
| Permission = apps.get_model("auth", "Permission") | ||
| ContentType = apps.get_model("contenttypes", "ContentType") | ||
|
|
||
| emit_post_migrate_signal(2, False, "default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be the cause of the failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this line? It should be removed.
| Permission = apps.get_model("auth", "Permission") | ||
| ContentType = apps.get_model("contenttypes", "ContentType") | ||
|
|
||
| emit_post_migrate_signal(2, False, "default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this line? It should be removed.
| log.info("Created 'project_managers' group.") | ||
| else: | ||
| log.info("'project_managers' group already exists.") | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the group already exists, we should quit early (without adding the permission).
| if not created: | |
| log.info("'project_managers' group already exists.") | |
| return | |
| log.info("Created 'project_managers' group.") |
| ("auth", "0001_initial"), | ||
| ("contenttypes", "0001_initial"), | ||
| ("sites", "0001_initial"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these are needed?
| ("auth", "0001_initial"), | |
| ("contenttypes", "0001_initial"), | |
| ("sites", "0001_initial"), |
|
(removing existing approval for clarity) |
Fixes mozilla#3492 HTML atribute values were not being highlighted as placeables in the translation editor, even though the attribute names were highlihted. Added color to match the teal highlighting used in bracket and tags names.
Bumps [django](https://github.com/django/django) from 4.2.27 to 4.2.28. - [Commits](django/django@4.2.27...4.2.28) --- updated-dependencies: - dependency-name: django dependency-version: 4.2.28 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
Bumps [protobuf](https://github.com/protocolbuffers/protobuf) from 5.29.5 to 5.29.6. - [Release notes](https://github.com/protocolbuffers/protobuf/releases) - [Commits](https://github.com/protocolbuffers/protobuf/commits) --- updated-dependencies: - dependency-name: protobuf dependency-version: 5.29.6 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Previously, we were indiscriminately reseting failed checks every time the editor result changed, including at the initial load of a translation. This fix checks whether the current editor content differs from the initial loaded content before clearing the warnings.
Bumps [cryptography](https://github.com/pyca/cryptography) from 46.0.3 to 46.0.5. - [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst) - [Commits](pyca/cryptography@46.0.3...46.0.5) --- updated-dependencies: - dependency-name: cryptography dependency-version: 46.0.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Also included: * Unify border radius * Drop obsolete Machinery CSS
…t_managers_group.py
…t_managers_group.py
…t_managers_group.py
…t_managers_group.py
…t_managers_group.py
Fix #3417.
Creates a data migration file that checks if project_managers group exists; if not, it creates it. Checks if can_mange_project permission exists; if not, the migration file creates it. Finally, grant project_managers can_mange_project permission.