Prioritize Override in Boolean Filter #45261

epeach · 2022-03-10T22:35:21Z

We got feedback that our override filter wasn't working. https://studio.code.org/s/dance?songfilter=on. This should turn the filter on for students regardless of their age or whether they are signed in.

The boolean logic we were using was filterOff = signedInOver13 || signedOutOver13AndNotFiltered. Even when we were filtering (signedOutOver13AndNoteFiltered = false), if a student was signed in and over 13 (signedInOver13 = true), we would still turn the filterOff (filterOff = true || false = true). So this might have never worked in this case, but because we are running into issues with signedInOver13 at the moment (#45233), this case is particularly relevant now.

To fix, I separated how we are tracking the override with it's own session storage token, rather than tagging it on the to the age token. Now, regardless of user_type or under13, if the override is on, we have the filter on. If the filter is off, we then go into the rest of the boolean logic that looks at user_type and under13.

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…verride

breville · 2022-03-16T03:07:41Z

First, I made a matrix to help understand what's fixed here and elsewhere:

                          override                 no override

signed in < 13            broken (3)               broken (2)
signed in 13+             broken (1)               works (no filter)

signed out < 13           works (filter)           works (filter)
signed out 13+            works (filter)           works (no filter)

(1) Will be fixed by this PR
(2) Will be fixed by https://github.com/code-dot-org/code-dot-org/pull/45233
(3) Interestingly will be fixed by either PR

epeach · 2022-03-17T23:43:22Z

@breville - PTAL - thanks!

breville · 2022-03-18T02:40:19Z

apps/src/templates/AgeDialog.jsx

@@ -48,7 +49,8 @@ class AgeDialog extends Component {
    // If the song filter override has been turned on, set session storage
    // Dialog won't render
    if (queryString.parse(window.location.search).songfilter === 'on') {
-      this.setManualFilter();
+      this.props.storage.setItem(SONG_FILTER_SESSION_KEY, true);


Do we actually want this choice to remain in place for the session? What if the user wants to turn the filter override off again? I thought that relying on the URL parameter for each page load could be more appropriate.

I lean towards making it hard to turn the filter off. When the student goes through the "run -> success -> continue" path and is navigated to the next page through the success dialog, the url parameter is dropped. Storing the filter in the session cookie will ensure that if a teacher wants to enforce the filter in their classroom and directs students to "studio.code.org/s/dance/lessons/1/levels/1?songfilter=on" and ask them to play through the tutorial, the filter will remain on for tutorial. I believe the '?songfilter=off' part was cut from the original spec so that students wouldn't have an easy time of turning the filter off if they've been directed to turn it on.

I see. If we are changing the existing behavior it seems worth capturing that in the PR description. I'm a bit concerned about subsequent students on shared computers getting confused, but I guess we use session storage for the signed-out age selector so this is at least consistent.

I don't believe the original intended behavior has changed in regards to whether the filter override lives in a session. Previously, in setManualFilter, we were storing the override in the AGE_DIALOG_SESSION_KEY. This is also where we were storing whether a signed out user was over/under 13. This PR splits that behavior into two session keys, so we can more accurately differentiate between signedIn with/out override and signedOut with/out override.

breville · 2022-03-18T22:22:46Z

I found the code for filtering rather confusing and wonder if it might be worth a small tidy-up while here. The PR description inadvertently alludes to this complexity as it uses psuedo-variables such as signedOutOver13AndNotFiltered which are more descriptive than what the code actually contains.

I ended up having to make the table above to help think through each case.

Some ideas to simplify:

Rename setFilterStatus to something like getInitialFilterStatus. With its current name, it's not clear what its return value means.
Consider avoiding the use of negative variable names, specifically filterOff, which requires inverting thinking about positive cases.
Having a songFilterOn function and a filterOff variable is another example of having to repeatedly invert thinking.

epeach · 2022-03-18T23:47:59Z

@breville - PTAL - thanks! I updated some of the variables/function name to simplify logic and added extra comments. I think it reads better now, but let me know if you agree!

apps/src/dance/DanceVisualizationColumn.jsx

epeach · 2022-03-23T22:11:14Z

@breville - PTAL - thanks! Broke out that utils function and added a test, plus overall clean up.

apps/src/dance/DanceVisualizationColumn.jsx

apps/src/dance/songs.js

breville · 2022-03-23T23:53:51Z

It looks like the age filter UI test is failing for the last few commits.

epeach · 2022-03-24T20:48:38Z

@breville - PTAL - fixed the tests - thanks!

breville · 2022-03-26T00:37:42Z

apps/src/dance/DanceVisualizationColumn.jsx

+    // If user is signed out, query session key to determine age.
+    // Return false (no filter), if user is over 13.
+    if (userType === 'unknown') {
+      return !signedOutOver13();
+    }


Slightly confused by the comment & code layout here. Presumably userType === 'unknown' checks whether a user is signed out, and signedOutOver13() queries the session key? It could be worth shuffling a little to make these things more clear, e.g.

// If user is signed out... if (userType === 'unknown') { // ...call signedOutOver13() to query session key to determine age. // Return false (no filter), if user is over 13. return !signedOutOver13(); }

I think there's a second issue here: signedOutOver13() should probably be able to return true if the user is signed out and over 13, but it sounds like it isn't able to determine whether the user is signed out, so we have to wrap with this extra check first? It seems weird to split that logic across two places, and means the function isn't able to do everything you'd expect it could.

Interesting point on the second issue. After reflecting on it, I realized that the function name currently infers a lot, so I updated the name to more accurately represent what it does - not if the user has selected over13 in the age dialog - ageDialogSelectedOver13.

breville · 2022-03-26T00:38:42Z

apps/src/dance/DanceVisualizationColumn.jsx

+      return !signedOutOver13();
+    }
+
+    // User is signed in (student or teacher) and the filter is off.


By "filter is off" do we mean that the "filter override is not turned on"?

breville · 2022-03-26T00:39:56Z

apps/src/dance/DanceVisualizationColumn.jsx

+    }
+
+    // User is signed in (student or teacher) and the filter is off.
+    // Return true (filterOn) if the user is under 13. Teachers assumed over13.


super nit: I'd replace "filterOn" with something like "filter should be turned on" since a function doesn't need to know about specific variables outside of its scope.

breville · 2022-03-26T00:41:47Z

Thanks for doing the extra work to build getFilteredSongKeys. The accompanying unit testing looks great.

epeach · 2022-03-31T23:07:42Z

@breville - PTAL - thanks!

breville

Nice work!

Prioritize override in filter boolean

37d2d84

epeach requested a review from a team March 10, 2022 22:35

Erin Peach added 2 commits March 10, 2022 20:19

Merge branch 'dance-party-filtering-update' into dance-party-filter-o…

3829149

…verride

Add specific session storage for filter

0df4689

epeach changed the base branch from staging to dance-party-filtering-update March 11, 2022 20:31

epeach requested a review from a team as a code owner March 11, 2022 20:31

breville mentioned this pull request Mar 11, 2022

Use under_13 to determine Dance Party Age - Clean-up #45233

Merged

8 tasks

Base automatically changed from dance-party-filtering-update to staging March 17, 2022 20:38

Merge branch 'staging' into dance-party-filter-override

504ebdf

breville reviewed Mar 18, 2022

View reviewed changes

Simplify naming

6f73f4a

breville reviewed Mar 20, 2022

View reviewed changes

apps/src/dance/DanceVisualizationColumn.jsx Outdated Show resolved Hide resolved

breville reviewed Mar 21, 2022

View reviewed changes

apps/src/dance/DanceVisualizationColumn.jsx Outdated Show resolved Hide resolved

breville reviewed Mar 21, 2022

View reviewed changes

apps/src/dance/DanceVisualizationColumn.jsx Outdated Show resolved Hide resolved

Erin Peach added 2 commits March 23, 2022 14:35

Move logic out of return

cdfdc8e

Create util for filtering and add test

59566cb

breville reviewed Mar 23, 2022

View reviewed changes

apps/src/dance/DanceVisualizationColumn.jsx Outdated Show resolved Hide resolved

breville reviewed Mar 23, 2022

View reviewed changes

apps/src/dance/songs.js Outdated Show resolved Hide resolved

Erin Peach added 2 commits March 23, 2022 17:13

Apply PR Feedback

8ba18a6

Break out logic for signed in and signed out

aacfe3d

breville reviewed Mar 26, 2022

View reviewed changes

Improve comments and function name

fc83001

breville approved these changes Apr 1, 2022

View reviewed changes

epeach merged commit 391fdb3 into staging Apr 1, 2022

epeach deleted the dance-party-filter-override branch April 1, 2022 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize Override in Boolean Filter #45261

Prioritize Override in Boolean Filter #45261

epeach commented Mar 10, 2022 •

edited

breville commented Mar 16, 2022 •

edited

epeach commented Mar 17, 2022

breville Mar 18, 2022

epeach Mar 18, 2022 •

edited

breville Mar 18, 2022

epeach Mar 18, 2022 •

edited

breville commented Mar 18, 2022 •

edited

epeach commented Mar 18, 2022

epeach commented Mar 23, 2022

breville commented Mar 23, 2022

epeach commented Mar 24, 2022

breville Mar 26, 2022

epeach Mar 29, 2022

breville Mar 26, 2022

breville Mar 26, 2022

breville commented Mar 26, 2022

epeach commented Mar 31, 2022

breville left a comment

Prioritize Override in Boolean Filter #45261

Prioritize Override in Boolean Filter #45261

Conversation

epeach commented Mar 10, 2022 • edited

PR Checklist:

breville commented Mar 16, 2022 • edited

epeach commented Mar 17, 2022

breville Mar 18, 2022

Choose a reason for hiding this comment

epeach Mar 18, 2022 • edited

Choose a reason for hiding this comment

breville Mar 18, 2022

Choose a reason for hiding this comment

epeach Mar 18, 2022 • edited

Choose a reason for hiding this comment

breville commented Mar 18, 2022 • edited

epeach commented Mar 18, 2022

epeach commented Mar 23, 2022

breville commented Mar 23, 2022

epeach commented Mar 24, 2022

breville Mar 26, 2022

Choose a reason for hiding this comment

epeach Mar 29, 2022

Choose a reason for hiding this comment

breville Mar 26, 2022

Choose a reason for hiding this comment

breville Mar 26, 2022

Choose a reason for hiding this comment

breville commented Mar 26, 2022

epeach commented Mar 31, 2022

breville left a comment

Choose a reason for hiding this comment

epeach commented Mar 10, 2022 •

edited

breville commented Mar 16, 2022 •

edited

epeach Mar 18, 2022 •

edited

epeach Mar 18, 2022 •

edited

breville commented Mar 18, 2022 •

edited