-
Notifications
You must be signed in to change notification settings - Fork 4
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
Switch to FB session login, and record last launch time #2396
Conversation
this also re-enables some tests that were being skipped
this can be used to clean up old roots
collaborative-learning Run #13695
Run Properties:
|
Project |
collaborative-learning
|
Branch Review |
188211348-switch-to-fb-session-login
|
Run status |
Passed #13695
|
Run duration | 13m 59s |
Commit |
daa70c7a2f: remove unnecessary app react state
|
Committer | Scott Cytacki |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
4
|
Skipped |
0
|
Passing |
110
|
View all changes introduced in this branch ↗︎ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2396 +/- ##
==========================================
+ Coverage 85.85% 85.90% +0.04%
==========================================
Files 738 736 -2
Lines 37855 37816 -39
Branches 9635 9623 -12
==========================================
- Hits 32502 32486 -16
+ Misses 5046 5025 -21
+ Partials 307 305 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
This looks like a great simplification.
src/components/app.tsx
Outdated
public state: IState = { | ||
qaCleared: false, | ||
qaClearError: undefined | ||
}; |
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.
Is there a reason to keep IState
as an empty object?
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.
It should work fine without it. I'll remove it.
This is ready to be merged, but I'm going to wait until #2401 is merged and deployed. Otherwise we'll end up with a lot of extra realtime database roots. |
By switching from a Firebase local login to a session login, each new tab/window will get its own Firebase login.
The reason for switching this is:
The last launch time is recorded so a background script can be scanning the roots and delete ones that haven't been used in a while. The firebase function for this background script will be in a separate PR.
This PR will make
appMode=dev
be much less "sticky" than it used to be. Each time a new browser tab is opened a new dev root in Firestore and the Realtime database will be created. A separate PR will improve this by storing the dev root in local storage so it is "sticky" again. In fact this new approach should be more sticky than it originally was.The majority of the files changed are Cypress tests that no longer need to call
clearQAData
. Other Cypress changes:teacher_share_spec
now has some re-enabled tests. It needed to be more careful about which thumbnail it was checking the visibility/shared state of.datacard_merge_spec
was counting on clearQAData to remove the datacard tile within a single test. This was addressed by explicitly clearing the browser state before re-visiting the page. It could have also been fixed by splitting this into multiple tests. However we are still trying to minimize the number of Cypress tests we record in the cloud.Other changes:
index.tsx
was simplified by moving the appMode calculation inside of initializeApp.