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

Prioritize Override in Boolean Filter #45261

Merged
merged 10 commits into from
Apr 1, 2022
Merged
62 changes: 40 additions & 22 deletions apps/src/dance/DanceVisualizationColumn.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import PropTypes from 'prop-types';
import Radium from 'radium';
import {connect} from 'react-redux';
import i18n from '@cdo/locale';
import AgeDialog, {signedOutOver13} from '../templates/AgeDialog';
import AgeDialog, {signedOutOver13, songFilterOn} from '../templates/AgeDialog';
import {getFilteredSongKeys} from '@cdo/apps/dance/songs';

const SongSelector = Radium(
class extends React.Component {
Expand All @@ -17,7 +18,7 @@ const SongSelector = Radium(
setSong: PropTypes.func.isRequired,
selectedSong: PropTypes.string,
songData: PropTypes.objectOf(PropTypes.object).isRequired,
filterOff: PropTypes.bool.isRequired
filterOn: PropTypes.bool.isRequired
};

changeSong = event => {
Expand All @@ -26,6 +27,15 @@ const SongSelector = Radium(
};

render() {
const {
selectedSong,
songData,
enableSongSelection,
filterOn
} = this.props;

const songKeys = getFilteredSongKeys(songData, filterOn);

return (
<div id="song-selector-wrapper">
<label>
Expand All @@ -35,17 +45,14 @@ const SongSelector = Radium(
id="song_selector"
style={styles.selectStyle}
onChange={this.changeSong}
value={this.props.selectedSong}
disabled={!this.props.enableSongSelection}
value={selectedSong}
disabled={!enableSongSelection}
>
{Object.keys(this.props.songData).map(
(option, i) =>
(this.props.filterOff || !this.props.songData[option].pg13) && (
<option key={i} value={option}>
{this.props.songData[option].title}
</option>
)
)}
{songKeys.map((option, i) => (
<option key={i} value={option}>
{songData[option].title}
</option>
))}
</select>
</div>
);
Expand All @@ -67,27 +74,38 @@ class DanceVisualizationColumn extends React.Component {
};

state = {
filterOff: this.setFilterStatus()
filterOn: this.getFilterStatus()
};

/*
Turn the song filter off
*/
turnFilterOff = () => {
this.setState({filterOff: true});
this.setState({filterOn: false});
};

/*
The filter defaults to on. If the user is over 13 (identified via account or anon dialog), filter turns off.
*/
setFilterStatus() {
getFilterStatus() {
const {userType, under13} = this.props;

// Check if song filter override is triggered and initialize song filter to true.
const songFilter = songFilterOn();
if (songFilter) {
return true;
}

// userType - 'teacher', 'student', 'unknown' - signed out users.
// under13 - boolean for signed in user representing age category. Teacher assumed > 13.
const signedInOver13 =
this.props.userType === 'teacher' ||
(this.props.userType === 'student' && !this.props.under13);
const signedOutAge = signedOutOver13();
return signedInOver13 || signedOutAge;
// 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();
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.


// User is signed in (student or teacher) and the filter is off.
Copy link
Member

Choose a reason for hiding this comment

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

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

// Return true (filterOn) if the user is under 13. Teachers assumed over13.
Copy link
Member

Choose a reason for hiding this comment

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

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.

return under13;
}

render() {
Expand All @@ -112,7 +130,7 @@ class DanceVisualizationColumn extends React.Component {
setSong={this.props.setSong}
selectedSong={this.props.selectedSong}
songData={this.props.songData}
filterOff={this.state.filterOff}
filterOn={this.state.filterOn}
/>
)}
<ProtectedVisualizationDiv>
Expand Down
12 changes: 12 additions & 0 deletions apps/src/dance/songs.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,15 @@ export function parseSongOptions(songManifest) {
});
return songs;
}

// Given a songManifest, returns a list of keys that represent the songs to
// be displayed based on whether the song filter is on.
export function getFilteredSongKeys(fullSongManifest, filterOn) {
const allSongKeys = Object.keys(fullSongManifest);
if (filterOn) {
// Filter is on, only include songs that are not pg13.
return allSongKeys.filter(key => !fullSongManifest[key].pg13);
}
// Filter is off, all songs may be displayed.
return allSongKeys;
}
12 changes: 7 additions & 5 deletions apps/src/templates/AgeDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ import queryString from 'query-string';
* a different session storage key here.
*/
const AGE_DIALOG_SESSION_KEY = 'ad_anon_over13';
const SONG_FILTER_SESSION_KEY = 'song_filter_on';

export const signedOutOver13 = () => {
return sessionStorage.getItem(AGE_DIALOG_SESSION_KEY) === 'true';
};

export const songFilterOn = () => {
return sessionStorage.getItem(SONG_FILTER_SESSION_KEY) === 'true';
};

class AgeDialog extends Component {
state = {
open: true
Expand All @@ -35,10 +40,6 @@ class AgeDialog extends Component {
storage: window.sessionStorage
};

setManualFilter = () => {
this.setSessionStorage(false);
};

setSessionStorage = over13 => {
this.props.storage.setItem(AGE_DIALOG_SESSION_KEY, over13);
this.setState({open: false});
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@epeach epeach Mar 18, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@epeach epeach Mar 18, 2022

Choose a reason for hiding this comment

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

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.

this.setState({open: false});
}
}

Expand Down
31 changes: 31 additions & 0 deletions apps/test/unit/dance/songsTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import {expect} from '../../util/reconfiguredChai';
import {getFilteredSongKeys} from '@cdo/apps/dance/songs';

let testSongManifest = {
allAgesSong: {
text: 'Singer - Song For Everybody',
pg13: false,
url: 'singer_song'
},
olderStudentSong: {
text: 'Band - Song for Older Students',
pg13: true,
url: 'band_song'
}
};

describe('Dance Party Songs Utils', function() {
describe('Song Filtering', function() {
it('when filtering is on, should only return keys of songs that are not pg-13', function() {
let filteredSongs = getFilteredSongKeys(testSongManifest, true);
expect(filteredSongs.length).to.equal(1);
expect(filteredSongs[0]).to.equal('allAgesSong');
});

it('when filtering is off, all song keys are returned', function() {
let filteredSongs = getFilteredSongKeys(testSongManifest, false);
expect(filteredSongs.length).to.equal(2);
expect(filteredSongs[1]).to.equal('olderStudentSong');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Feature: Dance Lab Age Filter
And I see option "Synthesize" or "Ed Sheeran - Shape of You" in the dropdown "#song_selector"

Then I am on "http://studio.code.org/s/allthethings/lessons/37/levels/1?noautoplay=true&songfilter=on"
And I reload the page
And I wait for the page to fully load
And I wait until I don't see selector "#p5_loading"
And I wait for the song selector to load
Expand Down