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

Fix #5260, query user status from server #5297

Merged
merged 3 commits into from Jan 10, 2019

Conversation

@ianb
Copy link
Contributor

commented Jan 9, 2019

Also fix #5263, remove My Shots links for non-server-users

Fix #5261, pop open server page for people with indefinite shots

This was a bit of an annoying patch, as anything involving state, several conditionals, timing, and failure conditions of all the above will be. This doesn't include a fix for #5262, but that will be a fairly simple addition to this change.

@ianb ianb requested a review from 6a68 Jan 9, 2019

@ianb ianb referenced this pull request Jan 9, 2019
// Wait until this many milliseconds to check the server for shots (for the purpose of migration warning):
const CHECK_SERVER_TIME = 10000; // 10 seconds
// If we want to pop open the tab showing the server status, wait this many milliseconds to open it:
const OPEN_SERVER_TAB_TIME = 10 * 60 * 1000; // 10 minutes

This comment has been minimized.

Copy link
@ianb

ianb Jan 9, 2019

Author Contributor

#5298 is a followup to determine the best time value.

// web ext background scripts don't send headers
return origin === undefined && referer === undefined;
// web ext background scripts don't send headers, or send a moz-extension origin
return (origin === undefined || origin.startsWith("moz-extension:")) && referer === undefined;

This comment has been minimized.

Copy link
@ianb

ianb Jan 9, 2019

Author Contributor

I don't know why this was different from other uses of /api/login (XHR vs fetch?), but for whatever reason I started seeing a moz-extension origin.

Fix #5260, query user status from server
Also fix #5263, remove My Shots links for non-server-users

Fix #5261, pop open server page for people with indefinite shots

@ianb ianb force-pushed the server-status-etc branch from 3bb867d to 829560f Jan 9, 2019

@6a68
6a68 approved these changes Jan 10, 2019
Copy link
Member

left a comment

Opening a tab when the user's in the middle of some other task seems kind of aggressive / hostile to me. I'd suggest just firing a desktop notification at startup, with no delay.

Editorial comments aside, LGTM as written, though QA should test the flow carefully, particularly with no network, to make sure the error cases work out ok.

@ianb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

We'll discuss the popup question in #5298

I'm landing this despite the test failure [I've disabled the bad test] (which I can't quite figure out – in one situation the autoselection isn't working, even though it's working elsewhere). These tests will go away with #5256, which will happen very soon, so doing lots of work to fix them when they are going to be deleted or changed doesn't seem useful.

@ianb ianb merged commit 4bab7aa into master Jan 10, 2019

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@ianb ianb deleted the server-status-etc branch Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.