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

Refresh class roster plus refactor #2418

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

scytacki
Copy link
Member

@scytacki scytacki commented Sep 30, 2024

If a student is added to the class after CLUE has already launched, and then this new student runs CLUE and joins a group. The first student should see this new student.

To support this several changes were made:

  • when the class store is updated from the portal data, it is done "in place" now with applySnapshot.
  • the groups store was updated to use the MST environment system instead of handling this itself. Originally the MST system was avoided because we planning to drop MST for the Groups store. Now that we are syncing groups using MST is the best option.
  • the groups store is now sync'd "in place" using applySnapshot
  • the groups store now looks up the users in the class store instead of copying their initials and name. This way when a new student is in the groups and not the class store yet, we can just update the class store and the UI will update.
  • We used to skip unknown group users in some configurations. These unkown users are group users with ids that are not in the class store. Now we show them all of the time. They might be unknown because they are new or because they were removed from the class.
  • switch authAndConnect to async/await to make it easier to read
  • add a timestamp to the class store so we can tell when this data was fetched from the portal. This timestamp is compared with the group user creation time to tell if the user has been removed from the class.
  • add a portal service object. this provides a place store portal info like JWTs, and methods that use and update this info.
  • use the portal service to refresh the class info if necessary after updating the group info from Firebase.
  • update the logic for deciding which students to boot from a group when it is oversubscribed. If there is a user that is removed from the class boot them first.

Testing in demo mode:

It's possible to test some of this in demo mode. In demo a fake class is created for each tab that is running CLUE. This fake class is not shared with other tabs running in the same demo space and class. The fake class includes students with ids from 1 to 99. And it also includes the student that is in the URL. So if you launch CLUE with fakeUser=student:900, this student will be added to the class of this CLUE window but will not be added to the classes of any of the other CLUE windows. Additionally the fake class has a timestamp of Date.now(). These two "features" can be combined to simulate a new and removed user in the class.

New User

  1. open a tab with CLUE in demo mode with a student:1 that is not in group. If this student is in a group already leave the group by clicking on the group button on the top right of CLUE. You should be seeing the group chooser.
  2. open a second tab with CLUE in the same demo space and class with student:900. When this student:900 joins a group, the student:1 tab should see the group with a ** in it.

This is because student:900 doesn't exist in the fake class in the student:1 tab. And timestamp of the class in the student:1 tab is older than the "connected" group user timestamp for this student:900.

Removed User

  • open a tab with CLUE in demo mode with a student:900. Now join a group.
  • wait at least 1 second
  • open a second tab with CLUE in demo mode with a student:1. If student:1 is an group, leave the group. You should not see student:900 or ** in any groups.

This is because student:900 is not in the fake class of the student:1 tab, and the group user connected timestamp for student:900 is older than the timestamp of the fake class used by the student:1 tab.

Watch out when reloading tabs

If you reload the tabs used above, things will change. Each time the tab is reloaded the fakeClass timestamp is updated to the current time. And the group user connected timestamp for the current student is updated to the current time. So this will change the logic.

TODO:

  • hide removed users in the UI
  • kick out removed users first from groups when resolving conflicts
  • add mechanism to test new and removed users in demo mode

This uses applySnapshot to update the class users in place.
This will make code using these users more efficient since they can just observer the user properties for changes.
Original the MST environment was avoided because I was planning to
switch this store to just be MobX. However the group store needs to
be synced with the data from the database, and it is easier to do that
using MST than MobX.
This uses applySnapshot to sync the changes from the DB. This way the existing objects are updated instead of creating new ones.

I also missed a change in stores when committing the last set of changes.
This will allow us to update the users later and have the group views automatically update without having to also update the group user objects.

This also removes the option to skip unknown students. They are part of the group so we should show them even if we don't know who they are yet.
We use a timestamp for when the class information is retrieved from the portal. And compare this to the timestamp in the groupUser to figure out if this groupUser is older than the class info or newer than the class info.
The portal service object provides a place to store portal info like JWTs, and methods that use and update this info.

This new object is used to refresh the class info if necessary after updating the group info from Firebase.
@scytacki scytacki changed the base branch from master to 188355313-fix-cypress-test September 30, 2024 16:00
Copy link

cypress bot commented Sep 30, 2024

collaborative-learning    Run #13915

Run Properties:  status check passed Passed #13915  •  git commit 5eea6afa22: add test of removed users
Project collaborative-learning
Branch Review 188181646-update-class-roster
Run status status check passed Passed #13915
Run duration 14m 09s
Commit git commit 5eea6afa22: add test of removed users
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 3
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 112
View all changes introduced in this branch ↗︎

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 89.88764% with 27 lines in your changes missing coverage. Please review.

Project coverage is 86.26%. Comparing base (4b252fe) to head (5eea6af).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
src/models/stores/portal.ts 91.17% 9 Missing ⚠️
src/components/app.tsx 73.33% 8 Missing ⚠️
src/lib/db-listeners/db-groups-listener.ts 77.14% 7 Missing and 1 partial ⚠️
...rc/clue/components/teacher/teacher-student-tab.tsx 0.00% 1 Missing ⚠️
src/lib/auth.ts 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2418      +/-   ##
==========================================
- Coverage   86.27%   86.26%   -0.02%     
==========================================
  Files         741      742       +1     
  Lines       38220    38312      +92     
  Branches     9772     9792      +20     
==========================================
+ Hits        32976    33050      +74     
- Misses       4943     4962      +19     
+ Partials      301      300       -1     
Flag Coverage Δ
cypress-regression 78.10% <85.42%> (+0.01%) ⬆️
cypress-smoke 27.85% <49.39%> (+0.04%) ⬆️
jest 48.80% <75.53%> (+<0.01%) ⬆️

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.

Base automatically changed from 188355313-fix-cypress-test to master September 30, 2024 17:22
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.

1 participant