-
Notifications
You must be signed in to change notification settings - Fork 270
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
fix(notifs): Default colors off #660
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
I have to go for today -- if anyone is able to fix the tests, it should be simple enough and will help us get this out faster. |
I suppose that this is fine, but I'd prefer to simply fix the auth issue. Also, isn't #654 going to affect this? |
Which issue, exactly? The problem, I think, is that currently-issued credential don't have repo scope. I think that's independent of #654, right? |
Well, yes, but that PR will probably re-prompt everyone to sign in again and fix the issue by itself, right? |
No, the intent of #654 is to change how the auth happens, but it won't affect existing auth sessions. Just the workflow of issuing new ones. |
What I am most confused about is how the normal login, not through the token, picks up the |
Just looked into it... 🤦 It requests it in the URL! We might as well simply reprompt for a login if they don't have the repo scope, right? |
Yeah I think that's the best solution for now. See #624 (comment) Want to take a stab at that instead? |
Closing in favour of #673 |
Per #624, enabling colors for existing users is breaking notifications for those without the repo scope.
As a quick stopgap, this PR defaults colors to off and flags that re-auth is required.
Following this, we can add an error check that shows a message to the user when their request fails because they don't have the necessary scope. Alternatively, as #624 suggests, we can just log the user out in this case. When we've done that, we can re-enable colors as a default. I don't have time for this better fix today, so pushing this.
I recommend we release this as 4.4.1 to fix the immediate problem. WDYT @afonsojramos ?