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

Teacher Override - Song Filter #25705

Merged
merged 15 commits into from Nov 5, 2018
Merged

Teacher Override - Song Filter #25705

merged 15 commits into from Nov 5, 2018

Conversation

epeach
Copy link

@epeach epeach commented Oct 30, 2018

Part of Age-Filtering

Builds on: #25741

Appending songfilter=on to the URL will override filtering songs based on age. Persists as long as the url contains songfilter=on.

Includes tests.

@islemaster
Copy link
Contributor

Are we sure this is the right approach? Using the querystring to override song filtering seems too easy to bypass. How did we decide on this approach to this feature?

@epeach
Copy link
Author

epeach commented Oct 30, 2018

I think we do want it to be very easy to bypass so that teachers who would prefer all their students have access to only filtered songs can provide this link to their student. Spec here: https://docs.google.com/document/d/1GXo6xekcqjIQ-UGjxcGhfKxZexxxYofck8C_MLUppVM/edit#

@ryansloan - can you provide more context of how we arrived at this solution?

@ryansloan
Copy link
Contributor

@islemaster Yep, that's right - it's by design that it's easy to bypass. We want to give teachers who want to be in control of the filtering the ability to just write a different URL on the board

@islemaster
Copy link
Contributor

Having songFilter=off as a "hidden" feature to bypass song filtering makes sense to me. Having songFilter=on to enforce song filtering seems nearly useless, given how easily a student could discover, omit and/or change this setting once used, and given most teachers will want to use our shortcode <pegasus>/danceparty which doesn't pass queryparams through the redirect.

@islemaster
Copy link
Contributor

Maybe that's an important question: Is there hidden work here? Do we need to pass queryparams through that shortcode and/or from level to level in the script?

@epeach
Copy link
Author

epeach commented Oct 30, 2018

I see what you mean about how easy it is to bypass the filter. @ryansloan Is this something we could allow teachers to turn on/off for their entire sections from the teacher dashboard?

@ryansloan
Copy link
Contributor

Ok, I talked with LT about this. We are going to only let you override to turn the filter on.
songfilter=off should be a no-op.

@epeach
Copy link
Author

epeach commented Oct 31, 2018

@ryansloan So just to confirm, once you turn the filter on for yourself, there is no way to turn it off?

@ryansloan
Copy link
Contributor

@epeach and I chatted about this. We are not trying to build a robust filtering system, only something that teachers can opt to use if they want to in their classroom. They can enforce the content restrictions using the same classroom management practices they use for computer time in general to ensure students are using the URL. It is trivial to get around, and that's ok.

We don't want any browser/user to be "forever filtered". Can we expire the cookie with the session?

@epeach
Copy link
Author

epeach commented Nov 4, 2018

@islemaster PTAL, thanks!

@islemaster
Copy link
Contributor

I assume you didn't mean to check in a schema_cache.dump?

@@ -0,0 +1,10 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I also have no idea what this file is, but it's probably specific to your machine.

@@ -0,0 +1 @@
../../../.nvm/versions/node/v6.9.0/lib/node_modules/@code-dot-org/dance-party
Copy link
Contributor

Choose a reason for hiding this comment

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

And I'd remove this symlink too.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Some extra files that need to be removed, but otherwise the actual implementation here looks great! Nice work.

@epeach epeach merged commit 1347a9e into staging Nov 5, 2018
@epeach epeach deleted the teacher-override-song-filter branch November 13, 2018 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants