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

Hide menu for invited users #705

Merged
merged 2 commits into from
Jan 6, 2023
Merged

Conversation

phloux
Copy link

@phloux phloux commented Jan 5, 2023

@phloux phloux marked this pull request as ready for review January 5, 2023 14:53
@giomfo
Copy link
Contributor

giomfo commented Jan 5, 2023

@phlniji your change is ok
but I did some tests, and I discovered a bug:

  • the plus button is not correctly updated after a new login :
    • If I log in with an agent account after log out an external account the (+) is missing
    • if I log in with an external account after logging out an agent account: the (+) is visible with the 3 options

I think you need to update this (+) button on userSessionsServiceDidAddUserSession like you already did for the top left button

@phloux
Copy link
Author

phloux commented Jan 6, 2023

@phlniji your change is ok but I did some tests, and I discovered a bug:

  • the plus button is not correctly updated after a new login :

    • If I log in with an agent account after log out an external account the (+) is missing
    • if I log in with an external account after logging out an agent account: the (+) is visible with the 3 options

I think you need to update this (+) button on userSessionsServiceDidAddUserSession like you already did for the top left button

We already observe the UserSessionsService.didAddUserSession notification.
So I think the notification registration has been done too late, or there is an issue with the guard isViewLoaded from updateUI method.
I will check this.

Copy link
Contributor

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

LGTM
As we discussed it in direct, I let you have a look on the use case highlighted above. Because the probability for an actual user to observe it is very low, we will manage it only if a simple fix is possible

@phloux phloux requested a review from giomfo January 6, 2023 14:18
Copy link
Contributor

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

LGTM

@phloux phloux merged commit 2e403d8 into develop Jan 6, 2023
@phloux phloux deleted the phlpro/fix-invited-users-rights branch January 6, 2023 15:09
NicolasBuquet pushed a commit that referenced this pull request Jan 19, 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.

2 participants