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

Mastery Mismatch Fix #2354

Conversation

DXCanas
Copy link
Member

@DXCanas DXCanas commented Oct 5, 2017

Summary

Bad times were had in the making of this PR. Fixes #2253, resolves #2089, fixes #2087, fixes #2252

@DXCanas DXCanas requested a review from rtibbles October 5, 2017 02:11
@DXCanas DXCanas changed the base branch from l10n_release-v0.4.x to release-v0.4.x October 5, 2017 02:11
@DXCanas
Copy link
Member Author

DXCanas commented Oct 5, 2017

@rtibbles In my tests, clearing out the log objects at page load for the show*Content pages just broke everything until this change was made. Looks like just this change is sufficient, but we can do those wipes (like we talked about) for good measure if you like.

@codecov-io
Copy link

codecov-io commented Oct 5, 2017

Codecov Report

Merging #2354 into release-v0.4.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           release-v0.4.x    #2354   +/-   ##
===============================================
  Coverage           73.83%   73.83%           
===============================================
  Files                 145      145           
  Lines                4904     4904           
  Branches              544      544           
===============================================
  Hits                 3621     3621           
  Misses               1206     1206           
  Partials               77       77

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fba58a5...ac2857e. Read the comment docs.

@@ -323,10 +323,6 @@ oriented data synchronization.
this.$emit('stopTracking', ...args);
},
},
beforeDestroy() {
// Make sure any unsaved data is captured before tear down.
this.saveAttemptLogMasterLog();

This comment was marked as spam.

@rtibbles
Copy link
Member

rtibbles commented Oct 5, 2017

A clean state would be nice - but my main concern with the current implementation is that we could lose data (we have limited the amount of logging that occurs for performance reasons, so things could get lost by not doing a final save).

@DXCanas
Copy link
Member Author

DXCanas commented Oct 5, 2017

orderofevents

Just some confirmation of the race condition

…hout race conditions.

Wiping logging information clean on content page load for safety reasons.
@DXCanas
Copy link
Member Author

DXCanas commented Oct 5, 2017

@rtibbles updated!

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Code looks good, works for me too.

@rtibbles rtibbles merged commit 08276c0 into learningequality:release-v0.4.x Oct 5, 2017
@indirectlylit indirectlylit added this to the 0.4.6 milestone Oct 12, 2017
@DXCanas DXCanas deleted the deadly-student-credit-stealing-log-race-condition branch February 20, 2018 18:48
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.

4 participants