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

navbar: add cog menu #2471

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Conversation

jrcastro2
Copy link
Contributor

@jrcastro2 jrcastro2 commented Oct 4, 2023

@jrcastro2 jrcastro2 force-pushed the add-cog-menu branch 3 times, most recently from 38554c8 to 0d1aeab Compare October 4, 2023 15:24
@jrcastro2 jrcastro2 marked this pull request as ready for review October 4, 2023 15:25
"administration.users",
_("Manage user"),
order=2,
endpoint_arguments_constructor=lambda: {"q": f"id:{current_user.id}"},
Copy link
Contributor Author

@jrcastro2 jrcastro2 Oct 4, 2023

Choose a reason for hiding this comment

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

Comment for reviewers, in the issue is specified to use the email. However if the preferences of the visibility are not set to public we cannot find the user by using the email, therefore I added the id that will always find the user (when searching as admin)

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember if you are user manager (role) you are supposed to be able to access the email field regardless if the email is visible or not (that is why in the admins search we can see all the users' emails - to be verified).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can see all the users in the admin page but we cannot search by email unless they have set the Profile visibility to Public.

List all users

Screenshot from 2023-10-05 10-41-33

Search for user with Public profile

Screenshot from 2023-10-05 10-41-53

Search for user with hidden profile

Screenshot from 2023-10-05 10-42-02

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice finding! thanks for checking this.

in this case I think we are just missing User manager generator (or permission?) in a right place for searching emails:

https://github.com/inveniosoftware/invenio-users-resources/blob/master/invenio_users_resources/services/permissions.py#L39

due to https://github.com/inveniosoftware/invenio-users-resources/blob/dbb9126b725b26501f4921a7d73fe8e15a3d2ea8/invenio_users_resources/services/schemas.py#L48

I think it is better to handle it by permissions rather than trying to circumvent it via ID. maybe makes sense to discuss during the standup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The email is not indexed if the user profile is set to hidden, therefore no one can search for it as it's not available, agreed over chat that we will use the username instead, which unique and indexed all the time and only admins can search for it.

@jrcastro2 jrcastro2 force-pushed the add-cog-menu branch 2 times, most recently from 72f6110 to f103548 Compare October 6, 2023 08:43
@@ -126,6 +126,7 @@ def record_detail(
pid_value, record, files, media_files, is_preview=False, include_deleted=False
):
"""Record detail page (aka landing page)."""
record_owner = current_rdm_records.records_service.get_owner(pid_value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment for reviewers:This returns the User model object which has no dict representation. As I commented here, maybe we should consider creating an interface for the owner as it could potentially be something different than a user.

Copy link
Member

Choose a reason for hiding this comment

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

record is already resolved here, can't we resolve the owner directly?

const handleError = (errorMessage) => {
console.error(errorMessage);
this.setState({ error: errorMessage });
};

return (
<Grid columns={1} className="record-management">
{permissions.can_edit && !isDraft && (
{permissions.can_manage && (
Copy link
Member

@zzacharo zzacharo Oct 10, 2023

Choose a reason for hiding this comment

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

shouldn't be this can_moderate? Are we exposing the manage button to record owners/curators ?

@zzacharo zzacharo force-pushed the add-cog-menu branch 3 times, most recently from 3e80c8b to 3edbd0e Compare October 10, 2023 16:27
@kpsherva kpsherva merged commit c8bae98 into inveniosoftware:master Oct 11, 2023
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.

3 participants