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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggl Plan is missing from integrations page but the integration is on by default #1784

Open
nidarasheed opened this issue Jul 13, 2020 · 2 comments 路 May be fixed by #1850
Open

Toggl Plan is missing from integrations page but the integration is on by default #1784

nidarasheed opened this issue Jul 13, 2020 · 2 comments 路 May be fixed by #1850
Assignees
Labels

Comments

@nidarasheed
Copy link

  • OS version: 10.15.4
  • Browser version: 83, Chrome
  • Extension version: 1.64.1

Relevant integration (if any):

Toggl Plan

馃悰 Describe the bug

The integration is missing from integrations list, but it's on by default.

Expected behaviour

User should be able to enable/disable it, even if we decide to have it on by default.

Steps to reproduce

Steps to reproduce the behaviour:

  1. Go to Integrations page under Settings
  2. Look for Toggl Plan

Other details or context

Slack

@kornelijepetak
Copy link
Contributor

I've been researching into this extensively, trying the plan.toggl.com permissions thingie and I realized that maybe it is not really possible to handle it as easily.

The main issue is the fact that we are currently showing in the list everything except *.toggl.com. Well, plan.toggl.com is implicitly allowed by the required permission *.toggl.com.

I've discussed this a bit with @shantanuraj and he advised me not to change the urls in the manifests, because any changes in the manifest causes more thorough check by the stores. So we considered the option to expand the Plan's item on origins.js to include another boolean property that would be checked in code to explicitely include that item in the list, despite the fact that it's a toggl domain.

However, this can't be done.

The problem is that if we are requesting the browser to add an origin that is already covered by another origin, it will not be added to the list of origins.

I've tried

chrome.permissions.request({ origins: ['http://test-string.toggl.com/'] })

and when I later check the list with getAll(), mine is not added.
When I do a

chrome.permissions.request({ origins: ['http://test-string.toggl-test.com/'] })

(note the toggl-test), then all is well, it is added to the origins as expected.

That being said, the problem with plan.toggl.com is that, while it can be shown in the list of integrations origins, it can't be disabled, as it is implicitely used by *.toggl.com. I have found no way of knowing whether the user accepted it or not (aka should I check the checkbox), because that URL is never returned from getAll (because it is already covered by the default generic toggl one).

So, in the end, I think that permissions in the manifest need to be changed after all.

Especially now after rebranding, since the permission should not state *.toggl.com, but *.track.toggl.com, since the API is now on that URL. That would automatically solve the issue with the plan.toggl.com as it would no longer be automatically covered. However, I think we are doing some subdomain contracting which would need to be adjusted as well.

@shantanuraj
Copy link
Contributor

Well since we are are in the post-rebrand era and we already have the themed builds out, I'm willing to take a chance with a longer review by altering the manifest

Do you know what's the final verdict on the API and supporting v8 on the new domains, I saw that it might not be the case but in my test I can see the v8 API is accessible over track.toggl.com and is probably being used by button atm

ps. We'd need access to just track.toggl.com and toggl.com not *.track.toggl.com right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants