-
Notifications
You must be signed in to change notification settings - Fork 570
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(manifest): Update the extensions manifests to adhere to the rebranding URLs #1850
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't break things. But I feel like this needs another 👀
LGTM 🚀
Yes, I definitely want another review. If not several. This is a slippery slope. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On first run the extension seemed to work fine,
But since granted permissions aren't revoked on loading a new extension build.
I created a new Chrome profile to test the extension and couldn't get the login-flow to work as intended the login still completes due to shared cookies, but the post-login page isn't shown and the user will remain on the public-web page.
See more here
Also noted an additional code-change regarding the domain naming in origins list
if (process.env.DEBUG && domain.endsWith('toggl.space')) { | ||
domain = 'toggl.com'; | ||
if (process.env.DEBUG && domain.endsWith('track.toggl.space')) { | ||
domain = 'track.toggl.com'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -37,8 +37,7 @@ | |||
"open_in_tab": true | |||
}, | |||
"permissions": [ | |||
"*://*.toggl.com/*", | |||
"*://toggl.com/*", | |||
"*://track.toggl.com/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't finish the login flow, I believe we still need access to toggl.com
sans prefix
Look at these lines
https://github.com/toggl/toggl-button/blob/ce3fc5d34f317e3564ecbd82783bed2296498bab/webpack.config.js#L76
https://github.com/toggl/toggl-button/blob/16155d8560dfa2d9d6b1e550e6cc87fbebf989e8/src/scripts/components/LoginPage.tsx#L198
The extension needs access to toggl.com/track/toggl-button-login/
to communicate with it and be informed when the login completes
https://github.com/toggl/toggl-button/blob/1f335016dc614a5289eac5b471bd9fbc8c93de2f/src/scripts/content/toggl.js#L37-L48
But this content script won't work unless it has access to the public-web page
@shantanuraj it seems that the problem you have described happens on I was trying to find out which part of my PR breaks things, but as I was removing my changes, nothing different was happening. And then I tried doing this on
The popup is logged in, I can indeed start a TE, but the login page will just remain there, with my email/password still entered, just as you described. |
@kornelijepetak I tried the steps you described and shared my recording here I am not able to reproduce this behaviour on |
🌟 What does this PR do?
Updated URLs so that it correctly covers
track.toggl.com
after rebranding.🐛 Recommendations for testing
Test the hell out of this because it has a potential to blow up.
I've tried it locally and it all seems to work correctly, but please test it functionally and also let me know if there's anything amis in the code.
This also updates the manifests, so it will have to go thru the reviews on stores, right?
📝 Links to relevant issues or information
Incidentally also closes #1784 (make sure to read the comment)
🖼 The feelings towards this PR