Skip to content

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Oct 1, 2025

Summary

Add regression test for erroneous randomize validation on HTML5.
Make randomize no longer required.
Prevent contentnodes from having their kind updated after creation. Ensure randomize is set at exercise creation.
Add tests for the above behaviours.

References

Fixes #5417

Reviewer guidance

Upload an HTML5 zip. Ensure that it gets properly saved into the channel, and that the channel is then publishable.

I used Claude Code to write the tests here, and I don't think I had to modify them at all. Production code was also implemented by Claude Code, but I checked and reviewed every line closely, and it was in accordance with my specific implementation plan, I just verified that it did it as I intended.

Make randomize no longer required.
Prevent contentnodes from having their kind updated after creation.
Ensure randomize is set at exercise creation.
Add tests for the above behaviours.
Copy link
Member

@marcellamaki marcellamaki 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 look good, and confirmation manual QA on publishing and testing content in Kolibri checks out.

Image

I ran into some other weirdness around channel publishing as discussed in slack , but at this stage, I don't think it's related to these changes, at least not in a way I can figure out. So, I think this is okay to merge into hotfixes for Peter's review, and we can continue to investigate the other error I found separately


class ExtraFieldsSerializer(JSONFieldDictSerializer):
randomize = BooleanField()
randomize = BooleanField(required=False)
Copy link
Member

Choose a reason for hiding this comment

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

👍

)

# Prevent kind from being changed after creation
if self.instance is not None and "kind" in data:
Copy link
Member

Choose a reason for hiding this comment

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

While I do prefer the strict sense of rejecting any request that includes kind for an update, it seems less risky to just have this check whether kind included in the request is indeed different. I wouldn't expect the inclusion of kind in the request, considering how frontend syncing functions, but this strictness could cascade into more required changes if there are surprises.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, I can loosen it!

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

…anged, not just when included in the payload.
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Latest commit updates to loosen the check, but also make it more specific -- so only when the kind is changed via comparing kind to the kind included in the payload. I'm going to go ahead and merge so Peter can QA tomorrow, but if @bjester has any additional feedback about the approach we can do that in follow up.

@marcellamaki marcellamaki merged commit 5c25b76 into learningequality:hotfixes Oct 1, 2025
13 checks passed
@rtibbles rtibbles mentioned this pull request Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants