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

refactor firebase root id computation #2403

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

scytacki
Copy link
Member

@scytacki scytacki commented Sep 12, 2024

This PR started as a change to use a random id in local storage for the root id when the appMode is dev. However that approach couldn't be finished because the Firebase functions needed to be updated. That incomplete work was split out into its own PR: #2407

What remains in this PR is:

  • a refactoring of the code to get the root id which is used in both Firestore and the Realtime database.
  • relaxing the Firestore rules so we can implement the random id behavior in the future. The Realtime database have been relaxed like this since the beginning.

Historical Options
When this PR included the local random id, the following options were considered for dealing with the problem of the broken functions. See #2407 for more info.

There is not enough time to fix the functions, so we have to look at other options.

  1. Don't apply this PR, or apply a stripped down version of it that has the refactoring without the functional change. In this case appMode dev will function the same as appMode qa. A new firebase user will be created by each tab that is opened and will get its new root. A developer can't build up a collection of documents and users in Firebase. At least appMode dev won't be broken in weird ways.
  2. Apply it anyway. appMode dev will appear broken because of the functions problem. However developers can still use it to test many parts of CLUE, and build a collection of documents. And we can incrementally fix the functions as we go forward.
  3. Revert the whole change to the session authentication storage. This leaves us back without a good way of handling the firestore metadata documents created during tests. So it seems one of the first 2 options is better than this.

- needed to relax the security rules for the dev root
- update user icon roll over so devs know the root
- update firebase and firestore to share the same logic for computing the root id based on the appMode
Copy link

cypress bot commented Sep 12, 2024

collaborative-learning    Run #13764

Run Properties:  status check passed Passed #13764  •  git commit 62d0166478: remove the local storage dev root id
Project collaborative-learning
Branch Review 188211348-local-storage-dev-root
Run status status check passed Passed #13764
Run duration 13m 30s
Commit git commit 62d0166478: remove the local storage dev root id
Committer Scott Cytacki
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 4
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 110
View all changes introduced in this branch ↗︎

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.90%. Comparing base (36b3ee1) to head (2722e42).
Report is 51 commits behind head on master.

Files with missing lines Patch % Lines
src/clue/components/clue-app-header.tsx 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2403      +/-   ##
==========================================
- Coverage   85.91%   85.90%   -0.01%     
==========================================
  Files         736      737       +1     
  Lines       37816    37824       +8     
  Branches     9623     9624       +1     
==========================================
+ Hits        32490    32494       +4     
- Misses       5021     5025       +4     
  Partials      305      305              
Flag Coverage Δ
cypress ?
cypress-regression 77.37% <95.83%> (+<0.01%) ⬆️
cypress-smoke 28.07% <66.66%> (+<0.01%) ⬆️
jest 48.78% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scytacki scytacki marked this pull request as ready for review September 12, 2024 17:20
@scytacki scytacki marked this pull request as draft September 14, 2024 00:13
@scytacki scytacki requested review from kswenson and removed request for bgoldowsky September 14, 2024 00:13
@scytacki scytacki marked this pull request as ready for review September 16, 2024 13:33
@scytacki
Copy link
Member Author

@kswenson I added you as the reviewer of this one. You've used dev mode more than anyone else, so it seems right that you should weigh in on the options above.

Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 Looks good -- of the options you presented, this one seems the best to me:

or apply a stripped down version of it that has the refactoring without the functional change. In this case appMode dev will function the same as appMode qa. A new firebase user will be created by each tab that is opened and will get its new root. A developer can't build up a collection of documents and users in Firebase. At least appMode dev won't be broken in weird ways.

I think the need to build up dev documents over time is somewhat rare, and if needed one can always use demo for that. It'd be nice to fix the firebase functions to handle this case at some point, but having dev work like qa in the short term seems better than having it be broken in weird ways.

This dev root approach requires the firebase functions to be updated and there isn't time to do that, now. The refactoring is kept so applying the change will be easier in the future.
scytacki added a commit that referenced this pull request Sep 17, 2024
This puts a random id in local storage and uses it for the Firestore and Realtime database root ids.
This approach is currently broken though, because the Firebase functions have not been updated to use this same root id. The functions continue to use the Firebase user id instead.

See this PR for more info: #2403
@scytacki scytacki changed the title use local storage for the dev root id refactor firebase root id computation Sep 17, 2024
Copy link
Member

@kswenson kswenson left a comment

Choose a reason for hiding this comment

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

👍 LGTM

firestore.rules Outdated Show resolved Hide resolved
Co-authored-by: Kirk Swenson <[email protected]>
@scytacki scytacki merged commit b8ad146 into master Sep 19, 2024
12 of 13 checks passed
@scytacki scytacki deleted the 188211348-local-storage-dev-root branch September 19, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants