continue tries to move to last place we had progress #13668
Conversation
dashboard/app/models/user.rb
Outdated
| def next_unpassed_progression_level(script) | ||
| user_levels_by_level = user_levels_by_level(script) | ||
|
|
||
| script.script_levels.detect do |script_level| | ||
| user_level = last_progressed_level(script) |
There was a problem hiding this comment.
Given that we already did the DB query to get user_levels_by_level, is it possible to avoid the DB query for last_progressed_level? It may not be (may not make sense to), I just want to raise the question.
There was a problem hiding this comment.
In particular, can the code be refactored to define next_unpassed. If it is non-nil, return it (without calling last_progressed_level). If it is nil, compute last_script_level (with or without the DB query).
There was a problem hiding this comment.
Oops, never mind, next_unpassed uses script_level_index.
There was a problem hiding this comment.
One option would be to do something like this:
user_level = user_levels_by_level.values.flatten.sort_by!(&:updated_at).last
which avoids another db query, at the expensive of some complexity. The number of user_levels here should be on the order of ~100 at most, so I don't think we need to worry too much about the cost of sorting.
Thoughts?
dashboard/app/models/user.rb
Outdated
| end | ||
|
|
||
| # Returns the next script_level for the next progression level in the given | ||
| # script that hasn't yet been passed, starting its search at the last level we submitted |
There was a problem hiding this comment.
How does (if at all) this work if we are writing progress asynchronously? Do we care?
There was a problem hiding this comment.
I believe that historically we (a) only used async progress writing during HoC and (b) we chose not to use it during last HoC
In the case that we were to turn on async progress writing, this feature would not work as expected, but I don't think that is all that bad of a bad.
There was a problem hiding this comment.
Also used for migrations to the user_levels table.
There was a problem hiding this comment.
Meaning that if we had a migration in progress, we might get unexpected results here?
There was a problem hiding this comment.
Meaning that if we turn on async writes (because of a migration or otherwise), we might get unexpected results here. Granted, I'm not sure we care...just raising the question.
There was a problem hiding this comment.
I guess the other risk is that even once the migration is finished, last_modified might no longer represent the last level the user actually had progress on.
There was a problem hiding this comment.
That is less of a concern, I think, because the queue operates in a FIFO manner. So the ordering of the timestamps should at least be correct. Right?
There was a problem hiding this comment.
Cool. I think we care a little about getting unexpected results (because we're running a migration for example), but not a lot. I think the alternative would be to store this "last progress level" somewhere else, which would mean an extra write every time we made progress, which seems less palatable.
dashboard/app/models/user.rb
Outdated
| script_level_index = 0 | ||
| if user_level | ||
| last_script_level = user_level.level.script_levels.where(script_id: script.id).first | ||
| script_level_index = script.script_levels.find_index(last_script_level) if last_script_level |
There was a problem hiding this comment.
You can use last_script_level.chapter - 1 instead of doing a find_index here.
When you click "continue" on the unit overview page, the current behavior is that it looks for the first lesson in the script that you haven't passed (ignoring things like unplugged, hidden, etc.).
This is problematic in that if you choose to have you class skip a stage, continue will still keep trying to take you back to it.
What we'd instead like to do is to find the last level that you made any progress on, and then take you to the next level after that (again ignoring things like unplugged, etc.)