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

Show a level as in progress when a student starts interacting with it #36918

Merged
merged 4 commits into from Sep 25, 2020

Conversation

jmkulwik
Copy link
Contributor

@jmkulwik jmkulwik commented Sep 25, 2020

Non-validated levels currently never show an "in-progress" state, shown to the user as a green outline on the progress bubble. They are marked as "not started" until the user clicks the "complete" button. Once the "complete" button is clicked, the level transitions to the "complete" state, shown to the user as a filled-in green progress bubble. This would happen because no milestones were being recorded until the "complete" button was clicked.

Now, we record milestones on every (debounced) click of the reset button. With this change, we add an additional feature to this click - we record the level as "in-progress."

Background

We started recording milestones once per run-reset cycle as part of this PR: #36606

This task is part of the larger auto-validation feature (see link below).

In the past, we did not record intermediate milestones for non-validated levels because the DB load was too high on the UserLevels table. Now the load capacity has been significantly increased and milestones can be safely recorded every time the reset button is clicked.

The TestResult is set at -150 because any other state should be able to overwrite it. This is the bare minimum "in progress" state. It only indicates that the user has interacted with the level (by clicking the Reset button). Any other result will contain more information.

The TestResult that is passed to the DB gets calculated like this (pseudocode)

record Max(existingTestResult, newTestResult)

Therefore, -150 will always be overwritten by a higher result and will never overwrite a higher result.

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@jmkulwik jmkulwik requested review from a team, clareconstantine, cforkish, JamesDH96, mvkski, jamescodeorg and molly-moen and removed request for a team and JamesDH96 September 25, 2020 18:02
@@ -1860,7 +1860,8 @@ StudioApp.prototype.silentlyReport = function() {
this.report({
app: getStore().getState().pageConstants.appType,
level: this.config.level.id,
skipSuccessCallback: true
skipSuccessCallback: true,
testResult: TestResults.LEVEL_STARTED
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies if this is a dumb question, but why do we report LEVEL_STARTED on every report instead of reporting it once when the level starts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"when the level starts" = on initial page load?

Let me know if this answers your question:
Initially, we wanted to report milestones more often so we can get a more accurate idea of how much time a user is spending on a level (details here if helpful: #36606). This milestone report is intended to allow us to measure the time spent on the level.

However, when triggering this milestone report, we weren't recording any sort of testResult as part of the API call. Even though we now had a record that the user had started the level, that wouldn't translate back to the user as a green outline on their progress bubble.

Reporting the progress as "LEVEL_STARTED" once when the level starts would probably be a better overall design, but the existing convention from other milestone is to record a testResult on every API call, even if that testResult will end up as a noop action.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmkulwik i wonder if renaming the LEVEL_STARTED constant would make the more clear? right now it looks like a boolean. maybe INITIAL_TEST_VALUE or LEVEL_STARTED_VALUE or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense now, thanks for the explanation. I tried to think of another name but the best I could come up with was "REPORTED_PROGRESS". Not sure if that's better than "LEVEL_STARTED" or not, up to you.

Copy link
Contributor

@cforkish cforkish left a comment

Choose a reason for hiding this comment

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

LGTM! left a comment about the LEVEL_STARTED name but not a blocker for me.

Copy link
Contributor

@jamescodeorg jamescodeorg left a comment

Choose a reason for hiding this comment

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

LGTM

@jmkulwik
Copy link
Contributor Author

I also couldn't think of an immediately better name. I'll be working on the feature for a bit, so if something better comes up, I'll swap over.

@jmkulwik jmkulwik merged commit 00049ef into staging Sep 25, 2020
@jmkulwik jmkulwik deleted the in-progress-on-run branch September 25, 2020 23:39
@jmkulwik jmkulwik added the auto-validation https://docs.google.com/document/d/1hF-wvav0MXLnH49x0WBeO8TcUibMaCUegZiDr8-6l-c/edit# label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-validation https://docs.google.com/document/d/1hF-wvav0MXLnH49x0WBeO8TcUibMaCUegZiDr8-6l-c/edit#
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants