Refactor sectionProgressRedux - prefactor for time_spent addition to progress tab

[jmkulwik]

This is a prefactor for the time_spent task LP-1037

This prefactor is only intended to make sectionProgressRedux more usable and readable as a global state manager for sectionProgress. It only changes sections related to loadScript. This refactor is not intended to do the following:

1. Clean up the data processing logic flow
2. Split HTTP calls into their own api
3. Improve testing or documentation

Specifically, this prefactor aims to fix the following issues in sectionProgressRedux

1. Helper functions that would otherwise be private were being exposed because they were being triggered as dispatched actions
2. The redux store was being used to hold partially-processed non-global state regarding sections. At any time, the store could contain incomplete data. Access to this data was only being controlled by another state variable, isLoadingProgress.
3. Several util functions and const variables were in sectionProgressRedux - it was handling more than just redux actions and reducers.

These were fixed in the following way

1. The creation of a sectionProgressConstants.js file to contain shared variables
2. The creation of a sectionProgressLoader.js file to manage the logic around loading and processing sectionProgress data
3. The removal of several redux actions that managed only local, intermediate data
4. Redux actions are now only called with complete data

This is part of a larger effort to make various improvements to the progress tab to start displaying more data to teachers about their students' progress.

time_spent will allow teachers to see how long a student has been working on a level. This will help teachers identify which students are struggling. Additionally, it will help us identify which levels are difficult and which are easy.

Links

Testing story

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]

This looks like an improvement to what we had, thank you for being proactive about code quality! This diff is rather large - do you think it would have been possible to split it over multiple PRs, maybe one for each of the bullet items in the description? Smaller PRs are significantly easier to read and write good comments for. As is, im happy that this looks like its better than what we had though.

[jmkulwik]

This looks like an improvement to what we had, thank you for being proactive about code quality! This diff is rather large - do you think it would have been possible to split it over multiple PRs, maybe one for each of the bullet items in the description? Smaller PRs are significantly easier to read and write good comments for. As is, im happy that this looks like its better than what we had though.

Hmm yes and no. There are some smaller sections that could have been done as different PRs. i.e. moving the constants to a different file. But that change wouldn't have made much sense on its own. I can definitely go through and make that its own PR now.

Thinking through the bulk of the change (moving loadScript from redux to the loader), I think after finishing all the work, I could have gone back and restructured the order of changes to make them into smaller separate PRs, but I'm not confident I could have done that while making the change. This started as a side project during meetings week, and I attempted a few different approaches before landing on the one that worked. Had I known it would work when I started trying it, I could have structured the change into smaller PRs, but it wasn't until late in meetings week, when the bulk of the work was done, that I knew that approach would work. For this section of changes, I can go through and change it to separate PRs, but that would be a long process where I'd need to re-implement the change and make a new set of commits, each with its own re-written test suite.

[cforkish]

@jmkulwik i had the same high-level feedback as Max when i looked at this on Friday, but i wanted to spend some more time with it to see if i had any more useful feedback. however since i have an active interest in eventually doing a thorough audit of how we use redux, i concluded that to fully understand and appreciate these changes would be better left until i have specifically allocated time for that task. so my takeaway here is that it looks like an improvement, i see no cause for alarm, and since you're the one working with this code right now i trust your judgement that this prefactor will make the rest of your task easier.

tl;dr LGTM!