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

Exception handling #236

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

alexdryden
Copy link
Contributor

There are a small handful of error handling issues we have run into that I keep meaning to put into a pull request. But I think I may need a bit of guidance on how you prefer to manage exceptions. This draft starts with one of the issues we've run into.

Description

A super admin gets an error message when trying to add or modify a page for book where they are not an author:
Screenshot 2024-12-12 at 10 10 15 AM

For us, this happens most often when we are working with an author and demonstrating a feature.

Issue

  1. bc2b134
    It looks as if error is expecting a message but is being passed a XMLHttpRequest object. Elsewhere I see you access the error message using the responseJSON method when presented with a similar situation, and that is what I've done here.
    I think there are other error messages that follow this same pattern (e.g., attempt to print a XMLHttpRequest object)--is this the fix you'd like to see?

  2. 871e7c8
    The other issue is with the logic of this exception. It seems like super admins, who could add themselves to the book's users, should be able to make changes to any book. So, the second commit changes the exception logic to display the error only if the user is not a book author and is not a super admin.
    But let me know if this is an intentional move to provide a method that prevents super admins from accidentally making changes.

Currently the error message "You do not have permission to modify
this book" only shows up for admins.
@craigdietrich
Copy link
Collaborator

Wow, thanks for taking such a deep dive into this! It's been a nagging problem since Scalar's inception (getting a clear error message back when saving a page).

I think your fix (issue 1 above) is right on. Let's go ahead with that!

Issue 2, though, I think should be deleted. The Save API doesn't allow non-authors to edit book pages (a security feature, or so we thought at the time), so it would require an overhaul of the Save API to make this error message valid. I still think it makes sense to not allow super admins page-write privileges, because then we'd have problems with keeping track of the version creator ID. And I think it makes sense as a security feature, but I could be convinced otherwise.

How easy would it be to change this pull request to include #1 but not #2?

Thanks!!

@alexdryden
Copy link
Contributor Author

Great! No problem at all to update the PR. I'll go through and see if I can find/trigger the other instances that look like #1. and apply the same fix.

Thanks for the explanation on #2.! That makes a lot of sense. And I can see why admins might need to look at the edit view of a page where they aren't an author to help troubleshoot, but I wonder if the interface could somehow signal to them in those instances that they won't be able to make and save changes--maybe greying out the "Save" and "Save and View" buttons and/or a banner message when admins open an edit page for a book they don't have author rights to?

@craigdietrich
Copy link
Collaborator

I'd love for the interface to show that a super admin can't save a page, whether greying out the save buttons or perhaps showing a notice bar. I'll need to do some testing but I don't think that Scalar right now delivers enough data to the front-end for it to know if it's a super admin. (I think a super admin is just typed as an "Author" in the front end fields.) But I'll put this on my TODO list to sort it out. I'll probably put an "is_super" field in there then I can adjust the edit page notice bar accordingly. Great suggestion!

@craigdietrich
Copy link
Collaborator

@alexdryden Following up, I committed the change (b580191) to add a warning to the top of the edit page in the event one is a super admin but not an author of the book.

Super-not-author-warning

Let me know what you think?

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