8000 Check version in Attention Stream before sending new data by aaga · Pull Request #246 · mozilla-rally/rally · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Apr 21, 2023. It is now read-only.

Check version in Attention Stream before sending new data #246

Merged
merged 8 commits into from
Sep 15, 2022

Conversation

aaga
Copy link
Contributor
@aaga aaga commented Sep 15, 2022

Closes #204

@netlify
Copy link
netlify bot commented Sep 15, 2022

Deploy Preview for rally-prod canceled.

Name Link
🔨 Latest commit a408f98
🔍 Latest deploy log https://app.netlify.com/sites/rally-prod/deploys/63233e79f359c10008756763

@netlify
Copy link
netlify bot commented Sep 15, 2022

Deploy Preview for rally-staging canceled.

Name Link
🔨 Latest commit a408f98
🔍 Latest deploy log https://app.netlify.com/sites/rally-staging/deploys/63233e79feb0e80008478475

@aaga aaga requested review from rhelmer and thomik-corp September 15, 2022 14:13
@@ -267,16 +271,10 @@ export class Rally {

const data = userStudiesDoc.data();

if (data.enrolled && this._state !== RunStates.Running) {
if (data.enrolled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove running check here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah I agree, this was preventing resume() from being called if the extension was already running. We might want to consider moving to a proper state machine that checks this vs. doing it ad-hoc like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resume() already has this same check in it's implementation, so I figured it was redundant.

@aaga aaga merged commit d70770f into main Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude existing users from additional data collection in the Attention Stream extension
3 participants
0