Fix isNotStartedLevel #40555
Fix isNotStartedLevel #40555
Conversation
|
Side thought for the future: The logic for determining whether a level was started on the front-end seems overly complicated. Whether or not a level can be started seems like something we should be able to determine straightforwardly on the back-end and just tell the front-end. |
| @@ -107,6 +107,16 @@ def get_channel_for(level, user = nil) | |||
| channel_token&.channel | |||
| end | |||
|
|
|||
| # If given a level and a user, returns whether any work has been done | |||
| # on the level | |||
| def level_started?(level, user) | |||
There was a problem hiding this comment.
Should this be in user.rb or level.rb instead? Or somewhere else?
| # If given a level and a user and optional script, returns whether the level | ||
| # has been started by the user | ||
| def level_started?(level, user, script = nil) | ||
| if level.channel_backed? # maureen make sure user is present or this will fail |
There was a problem hiding this comment.
So... have you made sure the user is present? :-)
There was a problem hiding this comment.
This is why I leave notes for myself! Thanks for catching that, will update.
| @@ -107,6 +107,16 @@ def get_channel_for(level, user = nil) | |||
| channel_token&.channel | |||
| end | |||
|
|
|||
| # If given a level and a user and optional script, returns whether the level | |||
There was a problem hiding this comment.
Why is script optional?
Could you elaborate a bit on what we define as having "started" a level?
There was a problem hiding this comment.
I made the script optional because in the app_options method where level_started? is being used, we're checking for the presence of script several times. Which leads me to believe there is a case where we're getting app options without a script.
There was a problem hiding this comment.
Hmmm, not sure how best to handle this. For querying progress, I think we need to have both (since levels can be used in multiple scripts).
(Also, there was a second question about what it means to "start" a level above.)
There was a problem hiding this comment.
I updated the comment with more information about what we mean by "started". Honestly, I was just following how the previous code defined "started", so I had to dig into when a channel and user level are created to answer this question. For channel-backed levels, a channel is created as soon as the user visits the level page, so that's when it's considered "started". For other levels the user_level is created when progress is first saved, so that's when it's considered "started".
There was a problem hiding this comment.
For the optional script, I could make it required to get level_started data and then wait to see if anyone reports an issue? For displaying this banner, I can't imagine a case where we'd want it outside of the context of a script.
There was a problem hiding this comment.
Thanks for the additional comment. I think this is fine for now but may be something we revisit in the future to see if we want to make it more consistent. (After reading your comment, I'm now kind of wondering why the logic is even different for channel-backed levels. I'm guessing it has something to do with when we want to show this banner for template levels but it might be worth chatting with Amanda about this at some point to make sure that the behavior is what we want.)
No it will not show for non-app levels. The banner is only displayed in the CodeWorkspace component. |
The config value
isNotStartedLevelwas being set by assuming that every 'Gamelab', 'Applab', 'Weblab', 'Spritelab', 'Dance' was channel backed. The dance lab level/s/coursed-2020/lessons/7/levels/9is not channel backed so a channel didn't exist for the level and thereforeisNotStartedLevelwas being set to true incorrectly. This resulted in the teacher seeing the level not started banner.The changes in this PR rename
isNotStartedLeveltodisplayNotStartedBannerto more accurately represent the return value of the function. It also changes how we determine if the level has been started by checking for a channel if a level is channel backed and checking for a user_level for any other level.Before:
After:
Links
Testing story
Added a test for the new config value and tested locally.
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: