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 Template_Categories table #2193

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

whabanks
Copy link
Contributor

@whabanks whabanks commented Jun 11, 2024

Summary | Résumé

This PR implements the backend work for Template Categories per the Short code ADR.

Change Details

  • Add the template_categories table to the DB in a migration
  • Add the TemplateCategory model and associations with Template and TemplateHistory
  • Implement dao CRUD methods to interact with the table
  • New Admin API endpoints under /template-category
    • Create Category - /template-category
    • Get all Categories - /template-category
    • Get Category - /template-category/<template_category_id>
    • Get Category by template id - /template-category/by-template-id/<template-id>
    • Update Category - /template-category/<template_category_id>
    • Delete Category - /template-category/<template_category_id>?cascade=[True | False]
  • New Admin API endpoint to associate a template with a category: /service/<id>/template/<template_id>/category/<template_category_id>

Related Issues | Cartes liées

Test instructions | Instructions pour tester la modification

If you're using Postman, you can import this request collection to hit each new endpoint. I've included example payloads for create and update

Default generic Template Category id's you can use in the requests

    DEFAULT_TEMPLATE_CATEGORY_LOW = "0dda24c2-982a-4f44-9749-0e38b2607e89"
    DEFAULT_TEMPLATE_CATEGORY_MEDIUM = "f75d6706-21b7-437e-b93a-2c0ab771e28e"
    DEFAULT_TEMPLATE_CATEGORY_HIGH = "c4f87d7c-a55b-4c0f-91fe-e56c65bb1871"
  • Basic Create and Update
    • Create - /template/category
    • Update - /template/category/<template_category_id>

Getting a Category

  • Get a single category - /template-category/<template_category_id>
  • Associate template with a Category - /service/<service_id>/template/<template_id>/category/<template_category_id>
    • Associate a template with one of the generic categories listed above
  • Get category by template id - /template-category/by-template-id/<template-id>
    • Get the category by template id with the template you used in the above test

Getting all Categories

You'll need to create some new categories to fully test the gets with filtering.

  • A hidden category - associate it with an SMS template

  • 1 not hidden category - associate it with an email template

  • Get all categories - /template-category

  • Get all categories filter by hidden /template-category?hidden=[True | False | None]

  • Get all categories filter by template_type/template-category?template_type=[sms | email]

  • Get all categories filter by hidden & template type /template-category?hidden=[True | False | None]&template_type=[sms | email]

Deleting Categories

  • Delete category - No cascade - Category not associated with any templates
    1. First, add a new category
    2. Delete the category with /template-category/template_category_id?cascade=False
    3. Note that the delete was successful
  • Delete Category - No cascade - Category associated with a template
    1. Get the category_id from the earlier test where you associated it with a template
    2. Delete the category with /template-category/<template_category_id>?cascade=False making sure cascade=False or omitted entirely
    3. Note that you are prevented from deleting the category due to the association with a template
  • Delete Category - Cascade - Category associated with a template
    • Set cascade=True in the query string and try deleting the same category again
    • Note that the category was deleted, and the template was associated with the Generic Default Low category

Release Instructions | Instructions pour le déploiement

Reviewer checklist | Liste de vérification du réviseur

  • This PR does not break existing functionality.
  • This PR does not violate GCNotify's privacy policies.
  • This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
  • This PR does not significantly alter performance.
  • Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

app/models.py Fixed Show fixed Hide fixed
- Fix typos
- Update sqlalchemy model
app/models.py Outdated Show resolved Hide resolved
- Added template_category_id to the template_history table
- Adjusted model class name TemplateCategories -> TemplateCategory
- WIP: Added some basic tests for the TemplateCategory dao
- Added a schema for TemplateCategory
- Added from_json and serialize to the TemplateCategory model
- Added the template/category Blueprint
- Add some initial CRUD api endpoints
app/template/rest.py Fixed Show fixed Hide fixed
app/dao/template_categories_dao.py Outdated Show resolved Hide resolved
app/dao/template_categories_dao.py Outdated Show resolved Hide resolved
app/dao/templates_dao.py Show resolved Hide resolved
app/models.py Outdated
@@ -1179,6 +1213,7 @@ class Template(TemplateBase):

service = db.relationship("Service", backref="templates")
version = db.Column(db.Integer, default=0, nullable=False)
category = db.relationship("TemplateCategory", backref="templates")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change this to template_category. Ignore my relationship comment above, this should work!

app/models.py Show resolved Hide resolved
- Changed the category relationship in Template from TemplateCategory to template_category
- dao_get_template_category_by_template_id now simply selects the template, and returns the associated template_category instead of using an expensive join
- Moved the default categories into our config file
- Added dao_update_template_category
- Added dao_delete_template_category_by_id
- Added routes to: get and delete a template category
- Added a route to get all template categories
- Updated the migration file to use default category uuid's from the config file
app/dao/template_categories_dao.py Fixed Show fixed Hide fixed
app/dao/template_categories_dao.py Fixed Show fixed Hide fixed
- Squash more bugs
- Fix formatting
- Added a couple more tests for the filters on dao_get_all_template_categories
- Excluded the template_category table from deletion in notify_db_session to preserve the 3 generic template categories between test runs
- Fixed inserts in the migration, apparently alembic / sqlalchemy doesn't like multi-line f-strings
- Made a few tests shorter by excluding the description_en and description_fr columns as they are optional
- Allow passing of a uuid to dao_create_template_category
- Fixed issues with get_template_categories and delete_template_category filters / flags
- Added a fixture to re-populate the template_category table with generic categories and removed template_categories from the list of tables that are excluded from the post-test db clear
whabanks and others added 4 commits June 24, 2024 10:51
- Fix incorrectly named prop call in dao
- Added category as a declared attribute in TemplateBase model
- Added a proper route to get a category by template id
- Added a foreign key to templates_history for the category id
- Fix a couple more tests
@whabanks whabanks marked this pull request as ready for review June 24, 2024 17:10
- Tweak tests
- No longer excluding template_category_id in template_category_schema
- Updated migration revision as 0453 was added for pinpoint work
@whabanks
Copy link
Contributor Author

whabanks commented Jun 24, 2024

Question: Since Template.process_type acts as and override to the process_type from the category should we change this column to be nullable?

Just saw the AC in this card to do exactly this - I will make the changes. We will also need to update dao_delete_template_category_by_id.

- Remove NOT NULL constraint on templates.process_type
- Simplify the dao delete logic
tests/app/template/test_rest.py Fixed Show fixed Hide fixed
tests/app/dao/test_templates_dao.py Fixed Show fixed Hide fixed
tests/app/conftest.py Fixed Show fixed Hide fixed
tests/app/conftest.py Fixed Show fixed Hide fixed
tests/app/dao/test_template_categories_dao.py Fixed Show fixed Hide fixed
tests/app/dao/test_template_categories_dao.py Fixed Show fixed Hide fixed
tests/app/dao/test_template_categories_dao.py Fixed Show fixed Hide fixed
tests/app/dao/test_template_categories_dao.py Fixed Show fixed Hide fixed
tests/app/template/test_rest.py Fixed Show fixed Hide fixed
@@ -241,6 +335,7 @@ def create_sample_template(
subject_line="Subject",
user=None,
service=None,
category=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need template_category_id here instead of category?

assert dao_get_template_category_by_id(template_category.id) == template_category


@pytest.mark.parametrize(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I wanted to cut down the size of these parameters by creating the categories_to_insert in the test body and modifying the hidden field per test. When I did it this way, then between the tests the categories were not being removed from the test DB and the unique constraint on the name_en and name_fr was causing the tests to fail.

I can leave it as is or do something like set the name to a str(uuid) to get around the unique constraint and shrink the size of this behemoth.

Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal I don't think. Weird that it couldnt delete the test data though.

Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, I'll wait for Jumana for the sign off. I just approved your WAF update to fix the waffles issue - if you merge that one your tests should pass here I hope!

Deletes a `TemplateCategory`. By default, if the `TemplateCategory` is associated with any `Template`, it will not be deleted.
If the `cascade` option is specified then the category will be forcible removed:
1. The `Category` will be dissociated from templates that use it
2. The template is assigned a category matching the priority of the previous category
Copy link
Member

Choose a reason for hiding this comment

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

Tiny edit here, but this makes it sound like it picks any category at random as long it has the same priority, instead of what (i think) it actually does which is picks the default category that matches the same priority.

assert dao_get_template_category_by_id(template_category.id) == template_category


@pytest.mark.parametrize(
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal I don't think. Weird that it couldnt delete the test data though.

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

3 participants