Reorder Section Dropdowns #35615

cforkish · 2020-07-01T21:32:25Z

In the teacher dashboard, various dropdown menus were listing sections in different orders. The chosen fix was to have all dropdown menus list them in the same order as the table on the teacher homepage, which means by descending ID (from user's perspective, most recently created at the top). [see here]

The main fix was to explicitly sort by descending ID in sectionsForDropdown, which is where most of the dropdowns were getting their data. However, the TeacherDashboardHeader component was using the teacherSections.sections object from the redux store, and since the primary goal of this ticket was to standardize section ordering, the solution I chose was to update this component to get its sections from the sectionsForDropdown function as well, rather than duplicating sorting logic in the component.

The TeacherDashboardHeader component was explicitly filtering out hidden sections (in other components those were filtered out on the backend), so I moved the filtering into sectionsForDropdown and added includeHidden = true param so as not to break code that assumes it isn't doing any filtering.

Links

jira

Testing story

I have not yet started digging into any of our testing. If there are relevant tests that should be updated or added for this ticket, please let me know.

EDIT: Tests are failing so I'm learning about them now! Hopefully the unit tests will pass with latest commit, but still need to look into UI tests.

Reviewer Checklist:

Tests provide adequate coverage
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

mvkski · 2020-07-02T18:14:47Z

Taking a look. For starters, I think your idea to separate your .gitignore changes into a seperate PR is a good one, because it makes it so that if needed this PR can be reverted without reverting changes to the gitignore

clareconstantine

this looks good to me - congrats on your first "real code" PR!

Erin007

Hooray! Nice work, Charlie. Looks good to me.

mvkski · 2020-07-06T17:38:12Z

apps/src/templates/teacherDashboard/teacherSectionsRedux.js

+  onCourseOverview,
+  includeHidden = true
 ) {
-  return state.sectionIds.map(id => ({
-    id: parseInt(id, 10),
-    name: state.sections[id].name,
-    scriptId: state.sections[id].scriptId,
-    courseId: state.sections[id].courseId,
-    isAssigned:
-      (scriptId !== null && state.sections[id].scriptId === scriptId) ||
-      (courseId !== null &&
-        state.sections[id].courseId === courseId &&
-        onCourseOverview)
-  }));
+  return state.sectionIds
+    .map(id => ({
+      ...state.sections[id],
+      isAssigned:
+        (scriptId !== null && state.sections[id].scriptId === scriptId) ||
+        (courseId !== null &&
+          state.sections[id].courseId === courseId &&
+          onCourseOverview)
+    }))
+    .filter(section => includeHidden || !section.hidden)
+    .sort((a, b) => b.id - a.id);
 }


I reccomend having a seperate function that filters out hidden sections, rather than including that logic into this sectionsForDropdown function.

Is there an advantage to piggy-backing off of this existing sectionsForDropdown function, rather than writing a seperate helper function for TeacherDashboardHeader, or putting this data munging logic inside of TeacherDashboardHeader itself?

Here are some concerns that lead to this suggestion:

Concern 1.)
I am mildly concerned that we are passing in data with the isAssigned property to TeacherDashboardHeader, but as far as I'm aware not using that property there. Could this make debugging this component slightly more confusing? Data with this property would appear in developer tools when debugging, could that be distracting when debugging issues? Also, having this data but not using has (incredibly minor given the size of the data) performance implications.

Concern 2.)
Could having one function that is used by multiple separate drop-downs increase code complexity over time? TeacherDashboardHeader doesn't need to know about the logic behind isAssigned to work. Our other dropdowns using this function don't need to care about filtering out hidden sections. In the future, if we add another dropdown that needs something special for it, it may not care about either isAssigned or hidden sections. This seems less than ideal since anyone making changes to the dropdown in any of these three components would need to read this one function, and would have to take extra time to understand its complexity.

Concern 3.)
By adding another parameter to an existing function rather than creating a new function that just filters and sorts the data, you're doubling the amount of unit tests you need to fully test this function. Each additional parameter to a function multiplies the number of tests you need to fully test that function by the number of qualitatively different values that input can have. In this case its not so bad because of the specifics of how the function as currently written works (none of the parameters interact with each other) but I still think its worth noting because that could not be the case for this function in the future.

thanks for raising these concerns @mvkski. the intended advantage of piggy-backing off that existing function was to solve the sorting issue in a single place rather than everywhere we list sections, but it turns out that function was far from the primary method of listing sections in dropdowns that i assumed it to be. given that, i agree that there's no good reason to reuse it in the TeacherDashboardHeader, especially considering the concerns you raised.

digging through potential solutions that would solve the sorting issue in a single place, i determined that the amount of refactoring needed was out of scope for this story and would be better to address as part of our H2 performance improvements. so for now i just applied the simplest (though also least elegant) fix, which was to find everywhere (hopefully) that we present a user-facing list of sections, and call one of two new helper functions in teacherSectionsRedux. sometimes the sections that we need to sort are in dictionary form, and sometimes they're already in a list, so i factored out the actual sorting into a single function that operates on arrays so that the actual sort order is still only defined in a single place.

as for filtering, i'm not a big fan of the fact that the logical meaning of teacherSections.sections changes depending on context (sometimes filtered, sometimes not), but again, addressing that is out of scope for this task because we set those sections in so many ways throughout our code. it seems that in general the convention is to handle the filtering on the backend, and the TeacherSectionsHeader is just a special case where we want them filtered for the dropdown but still need the hidden ones in redux in case the selected section is hidden. so rather than creating a helper function for that i just put the filtering right in the component and added a comment about it being a special case.

again, thanks for raising these concerns, because my naive thinking that the sectionsForDropdown function was the standard access point for listing sections meant that my original PR had missed a number of other dropdowns that needed sorting. let me know what you think of the new approach!

clareconstantine

This looks really good! One change requested: adding back in the hidden mock section in TeacherDashboardHeaderTest.js. Thanks!

apps/src/templates/teacherDashboard/teacherSectionsRedux.js

apps/test/unit/templates/teacherDashboard/TeacherDashboardHeaderTest.js

clareconstantine

looks good!

mvkski

LGTM!

…o be consistent with teacher homepage - sort array by *descending* id in `sectionsForDropdown` - updated TeacherDashboardHeader gets sections from `sectionsForDropdown` to be consistent with other section dropdowns - moved filtering of hidden sections into `sectionsForDropdown`

…onent

…dHeader` since that function turned out to be far from the only access point for listing sections * reverted to explicitly filtering out hidden section in `TeacherDashboardHeader` (see comment) * added functions to convert a dictionary of sections to sorted array, and to sort and unsorted array of sections * applied sorting to all user-facing lists of sections that i could find (hopefully all!) * removed unused references to `state.teacherSections.sections` encountered along the way

… old section order, and added tests for new sorting functions

…ll names

…removed mention of ignoring hidden sections in dropdown render test.

cforkish requested review from clareconstantine, Erin007 and mvkski July 1, 2020 21:32

cforkish force-pushed the cforkish/LP-1456 branch 3 times, most recently from 4d30473 to 9c3a352 Compare July 2, 2020 20:28

clareconstantine approved these changes Jul 3, 2020

View reviewed changes

Erin007 approved these changes Jul 6, 2020

View reviewed changes

mvkski reviewed Jul 6, 2020

View reviewed changes

cforkish changed the title ~~cforkish/LP-1456~~ reorder section dropdowns Jul 6, 2020

cforkish force-pushed the cforkish/LP-1456 branch 3 times, most recently from 0340bdf to 78d33dd Compare July 9, 2020 20:10

cforkish requested review from clareconstantine and mvkski July 9, 2020 20:31

cforkish changed the title ~~reorder section dropdowns~~ Reorder Section Dropdowns Jul 9, 2020

clareconstantine suggested changes Jul 20, 2020

View reviewed changes

apps/src/templates/teacherDashboard/teacherSectionsRedux.js Outdated Show resolved Hide resolved

apps/test/unit/templates/teacherDashboard/TeacherDashboardHeaderTest.js Show resolved Hide resolved

cforkish force-pushed the cforkish/LP-1456 branch 2 times, most recently from 6a99027 to 08a50be Compare July 21, 2020 20:25

clareconstantine approved these changes Jul 21, 2020

View reviewed changes

mvkski approved these changes Jul 21, 2020

View reviewed changes

cforkish force-pushed the cforkish/LP-1456 branch 4 times, most recently from fc04ac7 to b40f0ed Compare July 27, 2020 22:07

cforkish added 4 commits July 28, 2020 13:12

updated TeacherDashboardHeaderTest to work with updates to the comp…

439078e

…onent

found one more place we need to sort sections, updated tests assuming…

49f3dd3

… old section order, and added tests for new sorting functions

cforkish added 2 commits July 28, 2020 13:12

updated single-char names in anonymous map/filter functions to use fu…

d019621

…ll names

added comment about mock section data being pre-filtered/sorted, and …

6fc8651

…removed mention of ignoring hidden sections in dropdown render test.

cforkish force-pushed the cforkish/LP-1456 branch from b40f0ed to 6fc8651 Compare July 28, 2020 20:25

cforkish merged commit 92289b6 into staging Jul 28, 2020

cforkish deleted the cforkish/LP-1456 branch July 28, 2020 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder Section Dropdowns #35615

Reorder Section Dropdowns #35615

cforkish commented Jul 1, 2020 •

edited

mvkski commented Jul 2, 2020

clareconstantine left a comment

Erin007 left a comment

mvkski Jul 6, 2020

cforkish Jul 9, 2020

clareconstantine left a comment

clareconstantine left a comment

mvkski left a comment

Reorder Section Dropdowns #35615

Reorder Section Dropdowns #35615

Conversation

cforkish commented Jul 1, 2020 • edited

Links

Testing story

Reviewer Checklist:

mvkski commented Jul 2, 2020

clareconstantine left a comment

Choose a reason for hiding this comment

Erin007 left a comment

Choose a reason for hiding this comment

mvkski Jul 6, 2020

Choose a reason for hiding this comment

cforkish Jul 9, 2020

Choose a reason for hiding this comment

clareconstantine left a comment

Choose a reason for hiding this comment

clareconstantine left a comment

Choose a reason for hiding this comment

mvkski left a comment

Choose a reason for hiding this comment

cforkish commented Jul 1, 2020 •

edited