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

Distributed error reporting: Setting up the database that stores all the errors #12250

Conversation

thesujai
Copy link
Contributor

@thesujai thesujai commented Jun 5, 2024

Summary

  1. Create a new errorreports app and add it to INSTALLED_APPS
  2. Add errorreports database name to the additional sqlite database
  3. Create an ErrorReportsRouter to ensure all the reported errors are stored in a separate database -> errorreports.sqlite3

References

Fixes #12244

Reviewer guidance

I created a TestModel in errorreports app, which I then migrated and tested in a shell that the creation of objects with that model is written to errorreports.sqlite3 and not any other databases.


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

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Jun 5, 2024
@akolson akolson requested review from rtibbles and akolson June 5, 2024 21:28
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Setup looks correct to me! manual tests checkout! @rtibbles any other thoughts?

Screenshot 2024-06-06 at 21 32 40

label = "errorreports"
verbose_name = "Kolibri ErrorReports"

def ready(self):
Copy link
Member

Choose a reason for hiding this comment

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

There's probably no use case for this method. However if there is, then its worth changing its body from ... to pass that is more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought both were the same thing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to pass!

Copy link
Member

Choose a reason for hiding this comment

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

Pass is more explicit. ... could mean a host of things.
It goes back to my earlier question. Do we really need the ready method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not now, some apps inside core had it defined this way with pass. So I just copy-pasted

Copy link
Member

Choose a reason for hiding this comment

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

I thinks it's best we remove it as it won't really be necessary.

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Hi @thesujai! Just 1 blocking change, but we should be good for a merge once update is made!

@akolson akolson self-requested a review June 11, 2024 11:23
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @thesujai!

@akolson akolson merged commit e5f2b4c into learningequality:distributed-error-reporting Jun 11, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants