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

Api 414 set up enums #745

Merged
merged 87 commits into from
Mar 1, 2024
Merged

Api 414 set up enums #745

merged 87 commits into from
Mar 1, 2024

Conversation

xlorepdarkhelm
Copy link
Contributor

@xlorepdarkhelm xlorepdarkhelm commented Jan 16, 2024

This represents the first in a massive cleanup effort for models.py. Simply: Changing all of the "constant" strings defined in models.py into enums in enums.py, and update all references to them.

  1. All strings made into Enums, stored in enums.py to remove the definitions from the bloated models.py.
  2. All table models defined in models.py have been updated to use the Enum definitions where necessary - in several cases, "helper" models have been completely replaced.
  3. All references to the old strings have been replaced with the requisite Enum. In some cases, this was needed to be factored correctly, as some strings were repeatedly used in several enum cases (like SMS_TYPE and EMAIL_TYPE), so these were checked on a case-by-case basis.
  4. All tests have been likewise updated to use the new enums.
  5. Migrations for all affected fields have been added.
  6. All literal strings for these values have been replaced in all areas of the code with Enum values.

This is a very large PR. Because these strings were, well, everywhere. But it helps with the cleanup of models.py to help organize how such things are implemented, standardizing them, and pulling the enums out of the file completely.

Issue: #414

Note: Do not close this issue when the PR is merged, as the issue will not be completed. This is only part 1.

Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Whew, there is a lot here @xlorepdarkhelm, thank you!

After doing a preliminary review of it all, I had a few questions as I went through for clarification, and noted a few spots that we'll need to pay close attention to in the future (webauthn, priority queue, letter processing, and one or two other things).

I also flagged spots that were caught up in the mix that have fake test data like bogus email and phone numbers that we'll want to probably change too to make sure we're clear of any potential testing snafus in the future.

Overall this looks great so far, thanks for taking the time to work through all of this! On to figuring out what's up with the tests...

app/dao/uploads_dao.py Show resolved Hide resolved
app/enums.py Outdated Show resolved Hide resolved
app/enums.py Show resolved Hide resolved
app/enums.py Show resolved Hide resolved
app/models.py Outdated Show resolved Hide resolved
tests/app/service/test_rest.py Show resolved Hide resolved
tests/app/service/test_service_guest_list.py Show resolved Hide resolved
tests/app/test_model.py Show resolved Hide resolved
tests/app/test_model.py Show resolved Hide resolved
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Signed-off-by: Cliff Hill <Clifford.hill@gsa.gov>
Copy link
Contributor

@terrazoon terrazoon left a comment

Choose a reason for hiding this comment

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

I left a couple questions, mostly just out of curiosity. There is one place where the permissions expected in a test change kind of randomly, but I assume there's a reason for it. Everything else seems to be in order.

app/dao/services_dao.py Show resolved Hide resolved
app/notifications/rest.py Show resolved Hide resolved
tests/app/dao/test_invited_user_dao.py Show resolved Hide resolved
@ccostino ccostino requested a review from a team February 28, 2024 22:28
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thank you, @xlorepdarkhelm, lots of good stuff here!

I've finished a second pass through and had a few more questions and wanted to double check some things, but nothing major stands out, nicely done!

I'll run through and test locally as well while you and others are following up here. :-)

app/clients/__init__.py Show resolved Hide resolved
app/clients/__init__.py Show resolved Hide resolved
app/dao/fact_notification_status_dao.py Show resolved Hide resolved
app/enums.py Show resolved Hide resolved
app/job/rest.py Show resolved Hide resolved
tests/app/dao/test_service_permissions_dao.py Show resolved Hide resolved
tests/app/service/test_rest.py Show resolved Hide resolved
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

I tested all of the following:

  • log in
  • send a message
  • create a template
  • copy a template
  • view a dashboard
  • download a report
  • send an invite
  • create a service
  • update service settings
  • click anything else you feel like - poked around various other spots on the site and repeated several actions in multiple services
  • sign out

And everything is working fine, nor did I see any errors show up in the logs (none that weren't my own doing anyway 😆). Awesome work @xlorepdarkhelm, thank you!

@ccostino ccostino merged commit 18dddf4 into main Mar 1, 2024
5 checks passed
@ccostino ccostino deleted the API-414_Set_Up_Enums branch March 1, 2024 16:53
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.

None yet

4 participants