Added new Cypress route to delete TC data by user_id #2483
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
whabanks
requested review from
smcmurtry and
andrewleith
and removed request for
smcmurtry
tests/app/dao/test_api_key_dao.py
Outdated
| @@ -64,7 +64,7 @@ def test_expire_api_key_should_update_the_api_key_and_create_history_record(noti | |||
|
|
|||
| def test_last_used_should_update_the_api_key_and_not_create_history_record(notify_api, sample_api_key): | |||
| last_used = datetime.utcnow() | |||
| update_last_used_api_key(api_key_id=sample_api_key.id, last_used=last_used) | |||
| update_last_used_api_key(sample_api_key.id, last_used) | |||
There was a problem hiding this comment.
The rate_limit_db_calls decorator is expecting params through args not kwargs so I adjusted the test here to not use kwargs instead of refactoring the decorator to check both.
There was a problem hiding this comment.
I'm having trouble getting cypress to work locally so maybe I am not the best person to review this.
One more general thought though - have we considered getting cypress to spin up a test database like we use with our unit tests? Writing new endpoints just to delete test data seems like a lot of work.
| @@ -169,6 +176,50 @@ def _destroy_test_user(email_name): | |||
| db.session.rollback() | |||
|
|
|||
|
|
|||
| @cypress_blueprint.route("/template-categories/cleanup/<user_id>", methods=["POST"]) | |||
| @fetch_cypress_user_by_id | |||
There was a problem hiding this comment.
it seems a bit unnecessarily complicated to use a decorator to get the user here - why not just get it inside the delete_template_categories_by_user_id function?
There was a problem hiding this comment.
As we write more endpoints to clean up entities by user_id we'll need to fetch and check the user each time. I figured instead of repeating that chunk of code in each method we could just decorate it and not worry about that detail in the future.
Alternatively we could write it as a normal method and just do user = get_user_by_id(user_id) happy to refactor if that seems simpler
There was a problem hiding this comment.
Yes I think that would be simpler but I will leave it up to you
| @@ -169,6 +176,50 @@ def _destroy_test_user(email_name): | |||
| db.session.rollback() | |||
|
|
|||
|
|
|||
| @cypress_blueprint.route("/template-categories/cleanup/<user_id>", methods=["POST"]) | |||
| @fetch_cypress_user_by_id | |||
| def delete_template_categories_by_user_id(user_id, user): | |||
There was a problem hiding this comment.
Will anything bad happen if there are still templates in the db that reference a template category we are deleting? Should this function include a check that there are no templates associated with the categories we are deleting?
There was a problem hiding this comment.
Yes! great question here. When we built template categories I wrote the delete method with a cascade parameter for this very purpose (testing specifically). From the method doc:
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 to one of the default categories that matches the priority of the deleted category
3. Finally the `Category` will be deleted
Yes, we did discuss it, and I'm not sure that we've ruled it out. I keep coming back to how nice and clean the unit tests are, in that they spin up their own temporary DB and then just kill it entirely when finished. Certainly a lot easier to do when you aren't trying to do an end-to-end test... If we could do this this it would mean the cypress tests in the PR would no longer run against staging, and instead they would run against an entire environment created inside github action runners (db, redis, api, and admin all inside github) I did experiment with this concept awhile back when trying to get cypress tests integrated into API, but it only included db+redis+api: |
Summary | Résumé
This PR introduces a new Cypress endpoint for cleaning up template categories by user_id and pairs with this admin PR
Test instructions | Instructions pour tester la modification
CI passes, once merged
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur