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

use assign_script #18452

Merged
merged 2 commits into from
Oct 18, 2017
Merged

use assign_script #18452

merged 2 commits into from
Oct 18, 2017

Conversation

Bjvanminnen
Copy link
Contributor

Realized in a discussion with Erin Bond today that when assigning a script to a section via the script overview page, assigned_at was not getting set.

This PR makes it so that we use student.assign_script instead of User.track_script_progress. Both potentially create a new UserScript, but this one sets assigned_at instead of started_at/last_progress_at. This is the expected behavior, and matches what happens when you assign a script via teacher-dashboard as well.

@islemaster
Copy link
Contributor

Looks like User.track_script_progress is not documented. What is the semantic difference between these two methods - when should we use one or the other? Should one of these be removed? Should UserScript.first_or_create be modified to always set assigned_at if a new record is created, or should this be baked into our schema?

@Bjvanminnen
Copy link
Contributor Author

My understanding is that track_script_progress is just meant to say "I did a level in this script" whereas assign_script is meant to say "I'm in a section and that section has been assigned to this script". I think therefore it's by design that only one sets assigned_at. I just hadn't done due diligence when initially writing this code and chose the wrong one (at the time I was just interested in seeing that a UserScript got created, not realizing there were different ways to do this).

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Sounds good. I'd recommend adding related comments to the two methods just to make this more obvious in the future.

@Bjvanminnen Bjvanminnen merged commit 5b4ae54 into staging Oct 18, 2017
@Bjvanminnen Bjvanminnen deleted the assignScript branch October 18, 2017 22:13
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.

None yet

2 participants