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

Add Document class for Regulations3k #6058

Merged
merged 13 commits into from
Oct 2, 2020
Merged

Add Document class for Regulations3k #6058

merged 13 commits into from
Oct 2, 2020

Conversation

alexm118
Copy link
Contributor

@alexm118 alexm118 commented Oct 1, 2020

Adds the model for our regulations3k document that we search on. This will allow us to create and index all regulations in ES7.


Additions

  • SectionParagraphDocument - Represents the data we load into elasticsearch for regulations.

How to test this PR

  1. Inside the python container of our local docker stack run cfgov/manage.py search_index --rebuild -f and all regulations are loaded into ES7 index regulations3k. Can be confirmed via the kibana dashboard.

Checklist

  • PR has an informative and human-readable title
    • PR titles are used to generate the change log in releases; good ones make that easier to scan.
    • Consider prefixing, e.g., "Mega Menu: fix layout bug", or "Docs: Update Docker installation instructions".
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines
  • Future todos are captured in comments and/or tickets
  • Project documentation has been updated, potentially one or more of:
    • This repo’s docs (edit the files in the /docs folder) – for basic, close-to-the-code docs on working with this repo
    • CFGOV/platform wiki on GHE – for internal CFPB developer guidance
    • CFPB/hubcap wiki on GHE – for internal CFPB design and content guidance

Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

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

One quick comment, but I love how simple and readable this is.

cfgov/cfgov/settings/local.py Outdated Show resolved Hide resolved
@@ -0,0 +1,42 @@
from unittest import TestCase
Copy link
Member

Choose a reason for hiding this comment

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

Would we want Django's TestCase here instead of unittest?

Copy link
Contributor Author

@alexm118 alexm118 Oct 2, 2020

Choose a reason for hiding this comment

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

Good Question, I hadn't considered the differences but upon some research I don't think we need Django's TestCase:

Using unittest.TestCase avoids the cost of running each test in a transaction and flushing the database, but if your tests interact with the database their behavior will vary based on the order that the test runner executes them. This can lead to unit tests that pass when run in isolation but fail when run in a suite.

This doesn't rely on a database, we are just defining the class and preparing the document in memory, so I think unittest.TestCase makes more sense here

Copy link
Member

@higs4281 higs4281 left a comment

Choose a reason for hiding this comment

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

One nit suggestion.

cfgov/cfgov/settings/test.py Outdated Show resolved Hide resolved
Co-authored-by: william higgins <higs4281@users.noreply.github.com>
Copy link
Member

@higs4281 higs4281 left a comment

Choose a reason for hiding this comment

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

I'd like to do some more testing of Haystack operations.

@alexm118 alexm118 merged commit 71b780d into main Oct 2, 2020
@alexm118 alexm118 deleted the regulations-document branch October 2, 2020 19:12
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.

3 participants