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

Resolve missing kind preventing successful sync of quiz logs and quieter sync logging #8592

Merged
merged 3 commits into from Nov 4, 2021

Conversation

bjester
Copy link
Member

@bjester bjester commented Nov 4, 2021

Summary

  • Updates logging API to set kind for ContentSessionLog
  • Adds minimum length validator to some non-blank char fields to ensure they're not blank
  • Reduces log messaging originating from sync

References

Addresses @pcenov's comment
Resolves #8394

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

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 changes look good - seems like Django is not entirely convinced that the migration for the logger app exists? Possible it's some weird Py2/3 discrepancy, so maybe try rerunning makemigrations with Python 2 and see if anything different pops up?

@bjester
Copy link
Member Author

bjester commented Nov 4, 2021

@rtibbles I didn't think it needed a migration, and thought I would see a warning running tests if it did, so I didn't initially commit a migration 👍

@rtibbles
Copy link
Member

rtibbles commented Nov 4, 2021

Ah - gotcha! Yeah, Django likes to add a migration for any model change, regardless of whether it actually affects the DB or not.

@bjester bjester merged commit 0be46de into learningequality:release-v0.15.x Nov 4, 2021
@bjester bjester added this to the 0.15.0 milestone Nov 4, 2021
@pcenov
Copy link
Member

pcenov commented Nov 5, 2021

Hi @bjester, while testing this I noticed that only the first part of my comment is fixed with this PR namely a Coach is now able to see the scores of a learner and any questions answered.
At the same time my second point is still valid: I imported a full 0.14 facility to a 0.15 one and synced it. When I go to check the scores of a learner as a Coach the page with the quiz scores is not displayed and there's an error in the console: Uncaught (in promise) TypeError: currentAttempt.interaction_history.filter is not a function:

2021-11-05_16-53-55

@rtibbles
Copy link
Member

rtibbles commented Nov 5, 2021

@bjester - weird that interaction history is not an array, but perhaps we can coerce that in the API request, in the case that it is falsy for some reason?

@bjester
Copy link
Member Author

bjester commented Nov 5, 2021

Thanks for testing @pcenov! What instance was the quiz completed on? And what instance did you check the result as the Coach? I should have noted in the prior PR that we didn't make the quiz results compatible with older versions, so if the quiz was completed on 0.15 then I wouldn't expect it to show up correctly in 0.14, but the other way around should work appropriately

@pcenov
Copy link
Member

pcenov commented Nov 5, 2021

The quiz was completed on the 14.7 instance which was then imported in 0.15 where I was subsequently checking the results. So yeah, it seems that there is an issue indeed.

@bjester
Copy link
Member Author

bjester commented Nov 5, 2021

Okay yes that sounds like a bug. I will investigate.

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.

noisy syncing logs
3 participants