-
-
Notifications
You must be signed in to change notification settings - Fork 150
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
3175 courthouses - Add Courthouse Table to search #3184
Conversation
Update search model to add Courthouse table
Add Parent Court field Add Appellate Court field Add military jurisdictions
Add migration files for newly minted Courthouse and Update to Court Tables
Semgrep found 7
Avoid using null on string-based fields such as CharField and TextField. If a string-based field has null=True, that means it has two possible values for "no data": NULL, and the empty string. In most cases, it's redundant to have two possible values for "no data;" the Django convention is to use the empty string, not NULL. Ignore this finding from no-null-string-field. |
@mlissner I'm of the mind to ignore the semgrep warning here- as being inconsistent with our normal practice but I could change it. Otherwise- I think it should be ready for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, thank you!
Reflect changes to PR
@mlissner thank you for the suggestions. I believe this should address them. |
Appease semgrep that wanted null on blankable char fields Update migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments. Two more:
- Why did you add indexes to a bunch of the fields?
- In general, the
help_text
field should be the first argument to a model field (or second when it's a FK or M2M field).
When I looked at the attorney address model that you pointed to I saw it had plenty of indexes and I thought I must have been missing something. So I followed the lead. |
@mlissner back to you. 🏓 I created a simple two list set for country codes because I didnt want to install Django-countries to do what you were discussing above. |
I merged this so it's in the code, but deploy will have to wait until tomorrow. |
Thank you |
PR Resolves the following Issues
Address #3175
This PR