nextstrain / nextstrain.org

The Nextstrain website
https://nextstrain.org
GNU Affero General Public License v3.0
87 stars 49 forks source link

Add context to console error on groups page #743

Closed victorlin closed 8 months ago

victorlin commented 8 months ago

Description of proposed changes

Current implementation means an error for the settings page is shown in the console for every visit to a group page where you aren't logged in as an owner of the group.

It confused me until I realized that canUserEditGroupSettings is being called on the group page to determine whether to display the edit button. It seems like this is by design, and I'm not sure there is an easy way to achieve the same behavior without sending this call to /groups/{group}/settings. I think ok to leave as-is, but some extra context might be helpful for anyone who has a tendency to think that something is wrong when seeing red error messages in the browser console.

Related issue(s)

N/A

Checklist

jameshadfield commented 8 months ago

Some more context which wasn't apparent to me as someone who hasn't worked much on this page: Loading a group splash page will result in an OPTIONS request to (e.g.) https://nextstrain.org/groups/blab/settings/overview, which will return 401 (unauthorized), at least when not logged in. These errors are not possible for us to systematically hide from the browser console. This does seem like a design decision. In the case a user is not logged in it seems we have the available information to avoid the request in the first place, which would be a nicer approach.

tsibley commented 8 months ago

This seems fine… but also unnecessary?

…for anyone who has a tendency to think that something is wrong when seeing red error messages in the browser console.

Have you looked at the console of most any site with lots of JS recently? It's full of red errors. :joy: People don't care. :upside_down_face: And only devs will be looking at the console, so this is a pretty limited audience to start.

victorlin commented 8 months ago

@tsibley I probably could have worded it better - this is intended for devs like myself who don't understand that the error is by design without looking further. My thinking was that, since it was the only red error and seemingly coming from another page, it might actually be a bug. I looked into it only to realize that it's intentional.

I think some added context will potentially prevent others from going through the same process. It could be a comment in the file, but I figured an info message is more direct.