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

feat(interview): Project visibility [1] #1121

Merged
merged 10 commits into from
Dec 12, 2024
Merged

Conversation

jochenklar
Copy link
Member

@jochenklar jochenklar commented Aug 15, 2024

This PR adds a new visibility field to Project. private is the current behavior, internal allows everybody to access the project like a guest (read only). I added a choice field in case we need a public choice in the future.

At on point we probably want to restrict projects to groups/sites but this should be better done using yet another field.

This PR adds the Visibility model, which has a OneToOne relation to Project. If a project has a visibility it is visible to all users of the instance. Visibility has a sites and a groups field like e.g. Catalog to restrict this behavior.

The new ProjectUserFilterBackend allows for queries like /api/v1/projects/projects/?user=1 (used on projects) and returns all projects where the user (in this case 1) is a member or which are visible to them because of the visibility feature, but not because they are a site_manager or admin.

@jochenklar jochenklar added this to the RDMO 2.3.0 milestone Aug 15, 2024
@jochenklar jochenklar changed the base branch from interview to interview2 August 15, 2024 16:14
@jochenklar jochenklar changed the base branch from interview2 to interview August 15, 2024 16:15
@jochenklar jochenklar self-assigned this Aug 15, 2024
@jochenklar jochenklar changed the base branch from interview to interview-more August 16, 2024 15:32
@jochenklar jochenklar changed the base branch from interview-more to interview August 25, 2024 15:07
@jochenklar jochenklar force-pushed the interview-visibility branch from 199863c to be2dea7 Compare August 25, 2024 16:09
Copy link
Member

@MyPyDavid MyPyDavid left a comment

Choose a reason for hiding this comment

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

I think that the implementation so far looks good.

Our use case would probably require the site permissions as well.
Could we not make a rule based on user.role.member and project.owner.role.member to filter a bit for Site already? That when the current user is also a member of the same site as the project.owner?

@jochenklar
Copy link
Member Author

Yes, of course. But then you cannot create template projects for all sites right? We could add a choice same site or something like this. Or we add something in settings since you might want to prevent users to add projects to other sites. Actually we need to restrict the feature to site_admins anyway, since otherwise people could just spam other peoples projects list...

@MyPyDavid
Copy link
Member

Yeah maybe, but as long as Project does not have a sites field, the filtering could also be done just in the front-end on the projects table. Maybe users can select from which project.owners they want to see the shared projects to prevent spammable projects?
I thought this feature was meant for all users to use the project as template and not only for site_admins.

@jochenklar jochenklar changed the title Interview visibility [interview 1] Project visibility Nov 19, 2024
@jochenklar jochenklar changed the title [interview 1] Project visibility feature(interview): Project visibility [1] Nov 19, 2024
@jochenklar jochenklar changed the title feature(interview): Project visibility [1] feat(interview): Project visibility [1] Nov 19, 2024
@jochenklar jochenklar force-pushed the interview branch 2 times, most recently from da16d3e to 9c1477c Compare December 5, 2024 11:23
Base automatically changed from interview to 2.3.0 December 6, 2024 08:24
@jochenklar jochenklar force-pushed the interview-visibility branch from be2dea7 to 493f4b5 Compare December 6, 2024 08:53
@jochenklar jochenklar marked this pull request as ready for review December 6, 2024 16:05
@jochenklar jochenklar requested a review from MyPyDavid December 6, 2024 16:05

# @pytest.mark.parametrize('username,password', users)
# @pytest.mark.parametrize('project_id', projects)
# def test_project_update_visibility_get(db, client, username, password, project_id):
Copy link
Member

Choose a reason for hiding this comment

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

what about these tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I need to remove them, they moved to test_view_project_update_visibility.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@MyPyDavid
Copy link
Member

I think that "Manni Manager" is missing the role manager somehow from the test fixtures (maybe only multisite)

@jochenklar
Copy link
Member Author

I also changed the wording a bit:

image
image
image
image

and if sites or groups are enabled:
image

Copy link
Member

@MyPyDavid MyPyDavid left a comment

Choose a reason for hiding this comment

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

thanks! So that site managers can update the visibility!

approved when tests have passed

@jochenklar jochenklar merged commit 5d3bc73 into 2.3.0 Dec 12, 2024
19 checks passed
@jochenklar jochenklar deleted the interview-visibility branch December 12, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants