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

Catch inviting groups you can't access members of #4420

Merged
merged 1 commit into from Feb 2, 2024
Merged

Conversation

norbye
Copy link
Member

@norbye norbye commented Jan 30, 2024

Description

The feature that preloads users to allow them to be used as secretary before creating the meeting is beautiful with sudo permissions, but it crashes the page with a 403 error if you want to invite a group you don't have permission to load the members of (typically a group you're not part of)

Result

Works on staging after demoting myself. Now the little popup on the bottom left of the screen appears to inform the user that the members were not loaded, but that's all. If you invite some groups you're a member of and some you're not the functionality is still intact for the groups you have permission to access.

Testing

  • I have thoroughly tested my changes.

Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.


Resolves ABA-772

Copy link

linear bot commented Jan 30, 2024

@github-actions github-actions bot added the review-needed Pull requests that need review label Jan 30, 2024
@norbye norbye requested a review from a team January 30, 2024 01:19
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

That makes sense, I think

Comment on lines 149 to 156
.catch(() => {
// User has no permission to load group members
setFetchedGroupIds((prevIds) => [...prevIds, groupId]);
});
Copy link
Member

Choose a reason for hiding this comment

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

Should this be .finally()? It seems like that's what you "actually" want since you're not checking the error

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm it's a fair point.. the reason I put it in the catch clause is that I didn't want the 403 to pop up in the console as it's sort of expected behavior. But with the finally it would still show.

I guess it might be better to ignore 403 but throw all other errors tho - as that's the only expected one.

Open to other suggestions to solve the problem if you have any

@norbye norbye added approved Pull requests that have been approved and removed review-needed Pull requests that need review labels Feb 2, 2024
@norbye norbye merged commit 93699de into master Feb 2, 2024
4 checks passed
@norbye norbye deleted the meeting-invite-bug branch February 2, 2024 07:25
@ivarnakken ivarnakken added the bug-fix Pull requests that fix a bug label Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved bug-fix Pull requests that fix a bug
Projects
None yet
2 participants