Just use default lessonExtras unless explicitly set #45492

dmcavoy · 2022-03-25T02:48:53Z

Instead of needing to determine if lesson extras or tts autoplay are available for a unit when updating the assigned unit we can simply things by just using the default values unless the values are explicitly set as part of creating or updating the section. There is no harm in having lessonExtras be true for a section which is assigned to a course without lesson extras. It just means if a student were to go work on a different course that was not assigned to them they might see the lesson extras.

Testing story

Updated the unit tests

… provided

tess323 · 2022-03-28T23:52:23Z

not super versed in this area, but fine by me pending a check-in with LP engineers per Amanda's slack request

dmcavoy · 2022-03-29T00:11:04Z

Hey @code-dot-org/learning-platform I'm interested in hearing from you all about any risks you see with this change.

Here is what I'm thinking about. Right now you could have a suggestion that is assigned to Course D (which has lesson extras) and you could enable lesson extras when assigning that to the section. Then you could have students who are also working on Dance Party which does not have lesson extras. There is nothing I know of that breaks in that case and I would think that is a pretty common possibility of some CSF course being assigned but students doing other HOC tutorials.

Are there any things you can think of which I should check to make sure this change is safe?

I spoke with Amanda and she is fine with the default being updated to on.

davidsbailey

code changes LGTM. I'll let others settle the question of user-facing behavior.

dmcavoy · 2022-03-30T15:54:28Z

@jamescodeorg @maureensturgeon could I get one of your eyes on this today please? It is blocking the big assignment dropdown PR.

maureensturgeon · 2022-03-30T16:46:32Z

apps/src/templates/teacherDashboard/teacherSectionsRedux.js

@@ -875,14 +873,13 @@ export default function teacherSections(state = initialState, action) {
    }

    const lessonExtraSettings = {};
+    if (action.props.scriptId && !action.props.lessonExtras) {


I'm a bit confused about this case, if action.props.lessonExtras is false, we set lessonExtraSettings.lessonExtras to true?

Hmmm good call out. I was going for if lessonExtras was null so I can update to that

Thanks! I'm also slightly confused about why the unit tests were passing before, because it looked like that the case where action.props.lessonExtras is false was being tested. That might be worth looking into

jamescodeorg · 2022-03-30T16:46:50Z

Sorry I missed this -- could you elaborate a bit more on the motivation behind the change? The existing logic doesn't look too bad, does this change unlock other simplifications with the broader assignment dropdown work?

(It feels a little strange to me to have these settings set on a section that doesn't need them but the way we do section assignment is already strange so I don't mind the change.)

dmcavoy · 2022-03-30T17:15:36Z

Sorry I missed this -- could you elaborate a bit more on the motivation behind the change? The existing logic doesn't look too bad, does this change unlock other simplifications with the broader assignment dropdown work?

(It feels a little strange to me to have these settings set on a section that doesn't need them but the way we do section assignment is already strange so I don't mind the change.)

The reason I am exploring this change is because the way we currently are doing this requires that the Unit Overview and Course Overview page know about all the course offerings. This is because when you click the Assign button on those pages we need to know if the course has lesson extras in order to decide if we should set lesson extras to true for the section. Those pages especially the Unit Overview page already are slow to load so I was trying to cut down on the amount of information we were sending to those pages.

There is another way to get around this that I was considering if we don't want to move to this change. It was to pass down the lesson extras information for the course or unit to the page and then pass it through when assigning. Its just more information being passed around especially if the default we really want is people to use lesson extras unless they turn them off.

maureensturgeon · 2022-03-30T17:30:06Z

Its just more information being passed around especially if the default we really want is people to use lesson extras unless they turn them off.

Another option here would be to load this information async only when it's needed, if we want to pass less information from the backend on load

maureensturgeon · 2022-03-30T17:33:29Z

apps/src/templates/teacherDashboard/teacherSectionsRedux.js

@@ -875,14 +873,13 @@ export default function teacherSections(state = initialState, action) {
    }

    const lessonExtraSettings = {};
+    if (action.props.scriptId && !action.props.lessonExtras) {
+      lessonExtraSettings.lessonExtras = true;


Nit: for someone new coming in, it may not be clear why this is being set to true. I think keeping the variable defaultLessonExtras makes the intent more clear

jamescodeorg · 2022-03-30T17:37:02Z

The reason I am exploring this change is because the way we currently are doing this requires that the Unit Overview and Course Overview page know about all the course offerings. This is because when you click the Assign button on those pages we need to know if the course has lesson extras in order to decide if we should set lesson extras to true for the section. Those pages especially the Unit Overview page already are slow to load so I was trying to cut down on the amount of information we were sending to those pages.

Thanks for the explanation! I don't have a good picture of how this all works but naively, it seems like part of the problem is that the front-end is responsible for too much logic. It seems like it should be possible for the course and unit overview pages to just say "please assign this course/unit to this section" and the back-end would apply reasonable defaults for the other parameters. That said, I'm fine with this approach so that we can unblock the larger work and maybe revisit this later if we want to clean it up further.

jamescodeorg

+1 to Maureen's comments on lessonExtras and ttsAutoplayEnabled but otherwise LGTM.

dmcavoy added 2 commits March 24, 2022 22:45

Just use default lessonExtras unless explicitly set

8d8793b

Update lesson extras and tts auto play to set to default value if non…

eb2a2dc

… provided

dmcavoy added the self-paced-pl label Mar 25, 2022

Merge branch 'staging' into teacherSectionsRedux-updates

b584be7

dmcavoy requested review from fontie715, tess323 and a team March 28, 2022 19:37

dmcavoy requested a review from a team March 29, 2022 00:05

davidsbailey approved these changes Mar 29, 2022

View reviewed changes

maureensturgeon reviewed Mar 30, 2022

View reviewed changes

jamescodeorg approved these changes Mar 30, 2022

View reviewed changes

Check for undefined instead of false

cb12dc8

maureensturgeon approved these changes Mar 30, 2022

View reviewed changes

dmcavoy merged commit d1ecec1 into staging Mar 30, 2022

dmcavoy deleted the teacherSectionsRedux-updates branch March 30, 2022 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Just use default lessonExtras unless explicitly set #45492

Just use default lessonExtras unless explicitly set #45492

dmcavoy commented Mar 25, 2022 •

edited

tess323 commented Mar 28, 2022

dmcavoy commented Mar 29, 2022

davidsbailey left a comment

dmcavoy commented Mar 30, 2022

maureensturgeon Mar 30, 2022

dmcavoy Mar 30, 2022

maureensturgeon Mar 30, 2022

jamescodeorg commented Mar 30, 2022

dmcavoy commented Mar 30, 2022

maureensturgeon commented Mar 30, 2022

maureensturgeon Mar 30, 2022

jamescodeorg commented Mar 30, 2022 •

edited

jamescodeorg left a comment

Just use default lessonExtras unless explicitly set #45492

Just use default lessonExtras unless explicitly set #45492

Conversation

dmcavoy commented Mar 25, 2022 • edited

Testing story

tess323 commented Mar 28, 2022

dmcavoy commented Mar 29, 2022

davidsbailey left a comment

Choose a reason for hiding this comment

dmcavoy commented Mar 30, 2022

maureensturgeon Mar 30, 2022

Choose a reason for hiding this comment

dmcavoy Mar 30, 2022

Choose a reason for hiding this comment

maureensturgeon Mar 30, 2022

Choose a reason for hiding this comment

jamescodeorg commented Mar 30, 2022

dmcavoy commented Mar 30, 2022

maureensturgeon commented Mar 30, 2022

maureensturgeon Mar 30, 2022

Choose a reason for hiding this comment

jamescodeorg commented Mar 30, 2022 • edited

jamescodeorg left a comment

Choose a reason for hiding this comment

dmcavoy commented Mar 25, 2022 •

edited

jamescodeorg commented Mar 30, 2022 •

edited