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

Privacy Opt-Out not clickable #3254

Open
john43252345 opened this issue Mar 1, 2021 · 6 comments · May be fixed by #3818
Open

Privacy Opt-Out not clickable #3254

john43252345 opened this issue Mar 1, 2021 · 6 comments · May be fixed by #3818

Comments

@john43252345
Copy link

Describe the bug

Privacy Opt-Out not clickable

Accessed on Firefox 86.0 (64-bit)

@chrisgzf
Copy link
Member

Hi @john43252345, can I just check: did you have adblock turned on for NUSMods?

@chrisgzf
Copy link
Member

Thanks for the bug report @john43252345!

Triage Outcome

User Impact: Very low

This bug occurs in the privacy opt-out toggle in the settings page. It happens if adblock is turned on.

Cause of bug

Offending function:withTracker in onToggleTracking in SettingsContainer

Everything in withTracker only runs if matomo (the tool we use for analytics) is initialized. See bootstrapping/matomo.ts.

If adblock is turned on, matomo never initializes, and the opting out of tracking is deferred in a queuedTasks array, never to be run.

Steps to resolve

Difficulty: Low

  1. Don't use withTracker
  2. Write a useMatomo hook to get the matomo object.
  3. In SettingsContainer, check if matomo is initialized using useMatomo.
    • if not initialized: show opt-out by default
    • if initialized: use current behavior
  4. Disable the toggle if matomo is not initialized (so that adblock users can't interact with it)
  5. Refactor onToggleTracking to use matomo from useMatomo.
  6. Can some kind soul rename all instance of "mamoto" to "matomo" 😂 (sorry @ZhangYiJiang but your typos around the codebase kinda irks me slightly 😂)

Tests

(If you know how to write unit tests.) Write unit tests with RTL. Assert behaviour depending on whether the matomo object is present or not.

Testing

Take note that matomo is by default not initialized in the dev build. To properly test this, you might need to comment out the relevant code to ensure initializeMamoto() is run in the dev build in entry/main.tsx. You will also need an adblocker of your choice that allows you to toggle on and off. I personally use uBlock Origin. Up to you.

If anyone wants to take it up, do leave a comment here!

@riccqi
Copy link

riccqi commented Aug 9, 2021

Hi! I would like to take this up. Your steps to resolve were very helpful for me :)
I am a bit confused over the implementation of the useMatomo hook though. Since all we want is to get the matomo object, would the hook simply be:

//found in matomo.ts file

export function useMatomo() {
//where matomo is defined in the same file, matomo.ts
  return matomo; 
}

Any file that would require this hook would then just import in from matomo.ts, instead of having this hook in the src/views/hooks folder

Thanks!

@yaofeng-wang
Copy link
Contributor

Hi @riccqi, do you intend to resolve this issue? If not, I would like to take this :)

@riccqi
Copy link

riccqi commented Oct 16, 2021

Hi @riccqi, do you intend to resolve this issue? If not, I would like to take this :)

I was intending to, but was stuck in the process and haven't gotten a reply from the maintainers. Feel free to take over this issue :)

@yk-tuturu
Copy link
Contributor

Hi, I will try to tackle this issue

@yk-tuturu yk-tuturu linked a pull request Sep 4, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants