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

EQM Lesson regression fixes #12354

Merged

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented Jun 24, 2024

Summary

Fixes #12349 -- Cannot set lesson to visible (9fc128d)

  • Reverts to use is_active in validated_data.get() call introduced in ac17541

Fixes #12351 -- Cannot preview content in lesson creation (15d63b0)

  • Ensures lessonRoot handler only does Exercise things when there is an exercise

Reviewer guidance

@pcenov - do these address the issues linked above?

@nucleogenesis nucleogenesis added the TODO: needs review Waiting for review label Jun 24, 2024
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Jun 24, 2024
store.commit('lessonSummary/SET_STATE', {
toolbarRoute: {},
// only exist if exercises
workingResources: null,
resourceCache: cache,
});

Copy link
Member Author

Choose a reason for hiding this comment

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

In case this looks mistaken I did add it on purpose to put visual space between the logical blocks a bit

Comment on lines +279 to +284
if (contentNode.assessmentmetadata) {
store.commit('lessonSummary/resources/SET_PREVIEW_STATE', {
questions: contentNode.assessmentmetadata.assessment_item_ids,
completionData: contentNode.assessmentmetadata.mastery_model,
});
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I initially considered just updating the lessonSummary/resources/SET_CURRENT_CONTENT_NODE handler to just set the state.preview value if/when these values are present since we're already sending it the contentNode but went with the path of least resistance here to avoid giving our neglected Vuex modules any false hope of being refactored in the future, for their fated doom is imminent.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Code changes make sense, just needs manual QA.

Adding a regression test to the lesson API endpoint would be good too @nucleogenesis.

@@ -127,7 +127,7 @@ def update(self, instance, validated_data):
# Update the scalar fields
instance.title = validated_data.get("title", instance.title)
instance.description = validated_data.get("description", instance.description)
instance.is_active = validated_data.get("active", instance.is_active)
instance.is_active = validated_data.get("is_active", instance.is_active)
Copy link
Member

Choose a reason for hiding this comment

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

Looking above, in the create method, validated data is just passed directly to the Lesson.objects.create method, so it does make sense that the validated data contains the attribute mapped to source.

@@ -267,18 +267,22 @@ function _prepLessonContentPreview(store, classId, lessonId, contentId) {
getParams: { no_available_filtering: true },
}).then(
contentNode => {
const assessmentMetadata = contentNode.assessmentmetadata;
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, yes, I think this was a result of the refactor that removed the assessment state mapper, which presumably returned an empty object when it wasn't an exercise.

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

LGTM - both issues are fixed now!

@rtibbles rtibbles merged commit a946b7c into learningequality:develop Jun 25, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: backend Python, databases, networking, filesystem... DEV: frontend TODO: needs review Waiting for review
Projects
None yet
3 participants