Only show repo sign off button to permissioned users #4541
Conversation
Jongmassey
force-pushed
the
Jongmassey/fix-workspace-repo-button-bug
branch
from
808ca6e to
aef123b
Compare
|
before:
after:
|
bug: workspaces with private repos will show the "admin section" to all users regardless of their permissions. cause: the display logic is to check that the repo is private OR the user has one of two relevant permissions. fix: this changes the display logic to check only that the user has one of two relevant permissions. The template contains logic to only display the repo visibility change button for private repos so it is not needed in the view.
Jongmassey
force-pushed
the
Jongmassey/fix-workspace-repo-button-bug
branch
from
76652a4 to
6ee97a8
Compare
| project_membership( | ||
| project=project, | ||
| user=user, | ||
| roles=[role_factory(permission=permissions.workspace_archive)], |
There was a problem hiding this comment.
nit: Another possible condition that will result in show_admin is permissions.workspace_toggle_notifications being enabled, which might also be worth testing.
| # this is what defines "private" | ||
| class AnotherFakeGitHubAPI: | ||
| def get_repo_is_private(self, owner, name): | ||
| return name.startswith("private") | ||
|
|
||
|
|
| show_admin = ( | ||
| can_archive_workspace or repo_is_private or can_toggle_notifications | ||
| ) | ||
| show_admin = can_archive_workspace or can_toggle_notifications |
There was a problem hiding this comment.
thought: I have a very limited understanding of roles and permissions. But, if I look at the current roles.py, it looks as if only ProjectDeveloper has either of workspace_archive or workspace_toggle_notifications. No other role does.
It does seem a little messy that we're checking for either of these two permissions instead of a single role. But I guess that type of duplication could be cleaned up in the forthcoming work on improving roles and permissions.
(It's fine to leave this for now; it's more a general observation on the current state of roles and permissions.)
There was a problem hiding this comment.
I had similar thoughts and backed out of any major changes on the basis we're taking a more holistic look at this and it's likely to change more majorly quite soon
| # a workspace with a "private" repo | ||
| workspace = WorkspaceFactory( | ||
| project=project, repo=RepoFactory(url="http://example.com/repo/private1") | ||
| ) |
There was a problem hiding this comment.
thought: Without actually trying anything out, I think that regardless of the repository being private or public, it shouldn't make any difference in the test to whether show_admin is enabled — which is what we're testing. That is, the outcome of the test is now primarily based on the user's permissions.
Where we would see a difference between a private and public repository is in the rendered template, where the visibility button will appear for private repositories.
(I'm not too sure how much we favour testing content of rendered templates. Though, we do test a rendered template elsewhere in these tests though.)
bug: workspaces with private repos will show the "admin section" to all users regardless of their permissions.
cause: the display logic is to check that the repo is private OR the user has one of two relevant permissions.
fix: this changes the display logic to check that the repo is private AND the user has one of two relevant permissions.