Tutorial explorer: sort by different displayweight for grade band #18637
Conversation
The tutorials are now sorted by different display weights depending on the selected grade. Specifically, grade "all" continues to use "displayweight", "pre" and "2-5" use "displayweight_k-5", "6-8" uses "displayweight_middle", and "9+" uses "displayweight_high". This also includes a migration to support the new columns in the HocTutorials gsheet.
| add_column :displayweight_high, String | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Are we able to have the migration as a separate PR. Asher's recommendation was that if at all possible, migrations and code that uses migrations should deploy in separate DTPs (in case something goes wrong with the migration).
There was a problem hiding this comment.
I thought about it and convinced myself it wasn't necessary here, but you've convinced me back. Will do :)
There was a problem hiding this comment.
Migration is here: #18654
| // right set of data to match the currently-selected grade. | ||
| if (sortBy === TutorialsSortBy.displayweight) { | ||
| if (grade === "all") { | ||
| sortByFieldName = "displayweight"; |
There was a problem hiding this comment.
Can we use the enum value here? (Adding the new values to the enum as well.)
sortByFieldName = TutorialsSortBy.displayweight;
...
sortByFieldName = TutorialsSortBy.displayweight_k5;There was a problem hiding this comment.
Good point. There's a slight disconnect because the enum is designed to correlate to values in the (temporarily removed) dropdown, versus the actual field names that we use. I think I'll just introduce a second enum to address this.
| /* | ||
| * The main tutorial set is returned with the given filters and sort order. | ||
| * | ||
| * Whether en or non-en user, this filters as though the user is of "en-US" locale. | ||
| */ | ||
| filterTutorialSet(filters, sortBy, orgName) { | ||
| const grade = filters["grade"][0]; |
There was a problem hiding this comment.
Tiny nit: filters.grade[0]. Or better yet, can filters be an instance of a class that defines these as instance properties or methods?
class Filters {
constructor(grade, student_experience, activity_type, ...) {
this.grade = grade;
// ...
}
matchesFilter(tutorial) {
// Logic from static filterTutorials.
}
}There was a problem hiding this comment.
Good idea to build a class, but given how deeply filters are used throughout the tutorialExplorer, I'd like to do that in a future PR.
| alter_table(:tutorials) do | ||
| add_column :displayweight_k5, String | ||
| add_column :displayweight_middle, String | ||
| add_column :displayweight_high, String |
There was a problem hiding this comment.
I know we've added some metrics for this information. Are we going to backfill it at all before it goes live, or just let it populate over time? Can we start these from the existing displayweight stats for the "all" group?
There was a problem hiding this comment.
We're going to start with the same ranking from the existing displayweight column for the "all" group and use it for every single grade band. We'll have the updated rankings for each grade band by the end of the week.
There was a problem hiding this comment.
Yes, and those values are already added to the gsheet so the data is ready to go.
…band Conflicts: pegasus/migrations/123_add_grade_displayweights_to_tutorials.rb
Also, the non-English tutorials use the provided default sort order.
|
Ready for merge if anybody would like to approve... |
The tutorials are now sorted by different display weights depending on the selected grade.
Specifically, grade
"all"continues to use"displayweight","pre"and"2-5"use"displayweight_k-5","6-8"uses"displayweight_middle", and"9+"uses"displayweight_high".The migration for the new columns in the HocTutorials gsheet is in #18654.