-
Notifications
You must be signed in to change notification settings - Fork 18
Added new Cypress route to delete TC data by user_id #2483
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
base: main
Are you sure you want to change the base?
Changes from all commits
d4c4bcd
b6dcc2a
a80e529
b2ed6a7
8868204
dfb1d51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import os | ||
from functools import wraps | ||
|
||
from flask import current_app, jsonify | ||
|
||
from app.models import User | ||
|
||
EMAIL_PREFIX = os.getenv("CYPRESS_USER_EMAIL_PREFIX", "notify-ui-tests+ag_") | ||
|
||
|
||
def fetch_cypress_user_by_id(func): | ||
"""A simple decorator to fetch a user by id and pass it to the decorated function. | ||
Useful to reduce boilerplate in the Cypress REST routes that delete by user id. | ||
""" | ||
|
||
@wraps(func) | ||
def wrapper(user_id, *args, **kwargs): | ||
user = User.query.filter_by(id=user_id).first() | ||
|
||
if not user: | ||
current_app.logger.error(f"Error: No user found with id {user_id}") | ||
return jsonify({"error": f"User id {user_id} not found"}), 404 | ||
|
||
return func(user_id, user, *args, **kwargs) # Pass user instead of email_name | ||
|
||
return wrapper |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,17 +11,21 @@ | |
from flask import Blueprint, current_app, jsonify | ||
|
||
from app import db | ||
from app.cypress.decorators import fetch_cypress_user_by_id | ||
from app.dao.services_dao import dao_add_user_to_service | ||
from app.dao.template_categories_dao import dao_delete_template_category_by_id | ||
from app.dao.users_dao import save_model_user | ||
from app.errors import register_errors | ||
from app.models import ( | ||
AnnualBilling, | ||
EmailBranding, | ||
LoginEvent, | ||
Permission, | ||
Service, | ||
ServicePermission, | ||
ServiceUser, | ||
Template, | ||
TemplateCategory, | ||
TemplateHistory, | ||
TemplateRedacted, | ||
User, | ||
|
@@ -141,6 +145,7 @@ def _destroy_test_user(email_name): | |
cypress_service.created_by_id = current_app.config["CYPRESS_TEST_USER_ID"] | ||
|
||
# cycle through all the services created by this user, remove associated entities | ||
|
||
services = Service.query.filter_by(created_by=user).filter(Service.id != current_app.config["CYPRESS_SERVICE_ID"]) | ||
for service in services.all(): | ||
TemplateHistory.query.filter_by(service_id=service.id).delete() | ||
|
@@ -156,6 +161,8 @@ def _destroy_test_user(email_name): | |
TemplateRedacted.query.filter_by(updated_by=user).delete() | ||
TemplateHistory.query.filter_by(created_by=user).delete() | ||
Template.query.filter_by(created_by=user).delete() | ||
TemplateCategory.query.filter_by(created_by=user).delete() | ||
EmailBranding.query.filter_by(created_by=user).delete() | ||
Permission.query.filter_by(user=user).delete() | ||
LoginEvent.query.filter_by(user=user).delete() | ||
ServiceUser.query.filter_by(user_id=user.id).delete() | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! great question here. When we built template categories I wrote the delete method with a
|
||
"""Deletes all template categories created by user_id. | ||
|
||
|
||
Args: | ||
user_id (str): The id of the user to delete template categories for. | ||
user (User): The DB user object to delete template categories for, fetched by the fetch_cypress_user_by_id decorator. | ||
|
||
Returns: | ||
A JSON response with a 201 if all template categories were successfully deleted. If a template fails deletion the ID | ||
is stored and returned with a 207 response. | ||
""" | ||
query = TemplateCategory.query.filter_by(created_by_id=user_id) | ||
results = query.all() | ||
remaining = [] | ||
|
||
current_app.logger.info(f"[Cypress API]: Deleting {len(results)} template categories created by user {user_id}.") | ||
|
||
for template_category in results: | ||
try: | ||
dao_delete_template_category_by_id(template_category.id, cascade=True) | ||
except Exception as e: | ||
current_app.logger.info(f"[Cypress API]: Error deleting template category {template_category.id}: {str(e)}") | ||
remaining.append(template_category.id) | ||
|
||
if remaining: | ||
message = ( | ||
jsonify( | ||
message=f"Template category clean up complete {len(results) - len(remaining)} of {len(results)} deleted.", | ||
failed_category_ids=remaining, | ||
), | ||
207, | ||
) | ||
else: | ||
message = ( | ||
jsonify(message=f"Template category clean up complete {len(results) - len(remaining)} of {len(results)} deleted."), | ||
201, | ||
) | ||
|
||
return message | ||
|
||
|
||
@cypress_blueprint.route("/cleanup", methods=["GET"]) | ||
def cleanup_stale_users(): | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,14 @@ | ||
import json | ||
import os | ||
from datetime import datetime, timedelta | ||
from unittest.mock import patch | ||
|
||
from app.models import User | ||
from tests import create_cypress_authorization_header | ||
from tests.app.conftest import create_sample_template, create_template_category | ||
from tests.conftest import set_config_values | ||
|
||
EMAIL_PREFIX = "notify-ui-tests+ag_" | ||
EMAIL_PREFIX = os.getenv("CYPRESS_USER_EMAIL_PREFIX", "notify-ui-tests+ag_") | ||
|
||
|
||
def test_create_test_user(client, sample_service_cypress): | ||
|
@@ -97,3 +100,43 @@ def test_cleanup_stale_users(client, sample_service_cypress, cypress_user, notif | |
|
||
user = User.query.filter_by(email_address=f"{EMAIL_PREFIX}[email protected]").first() | ||
assert user is None | ||
|
||
|
||
def test_delete_template_categories_by_user_id_success(client, cypress_user, notify_db, notify_db_session): | ||
cascade = "true" | ||
path = f"/cypress/template-categories/cleanup/{cypress_user.id}?cascade={cascade}" | ||
auth_header = create_cypress_authorization_header() | ||
|
||
category = create_template_category(notify_db, notify_db_session, created_by_id=cypress_user.id) | ||
|
||
with patch("app.cypress.rest.dao_delete_template_category_by_id") as mock_delete: | ||
mock_delete.return_value = None # Simulate successful deletion | ||
response = client.post(path, headers=[auth_header], content_type="application/json") | ||
|
||
assert response.status_code == 201 | ||
resp_json = json.loads(response.get_data(as_text=True)) | ||
assert resp_json["message"] == "Template category clean up complete 1 of 1 deleted." | ||
|
||
# Verify the mock was called for each template category | ||
assert mock_delete.call_count == 1 | ||
mock_delete.assert_any_call(category.id, cascade=True) | ||
|
||
|
||
def test_delete_template_categories_by_user_id_exception(client, cypress_user, notify_db, notify_db_session): | ||
path = f"/cypress/template-categories/cleanup/{cypress_user.id}" | ||
auth_header = create_cypress_authorization_header() | ||
|
||
# Mock template categories created by the user | ||
categories = [ | ||
create_template_category(notify_db, notify_db_session, name_en="1", name_fr="1", created_by_id=cypress_user.id), | ||
create_template_category(notify_db, notify_db_session, created_by_id=cypress_user.id), | ||
] | ||
create_sample_template(notify_db, notify_db_session, template_category=categories[0]) | ||
|
||
with patch("app.cypress.rest.dao_delete_template_category_by_id", side_effect=Exception("bad things happened")): | ||
response = client.post(path, headers=[auth_header], content_type="application/json") | ||
|
||
assert response.status_code == 207 | ||
resp_json = json.loads(response.get_data(as_text=True)) | ||
assert resp_json["message"] == "Template category clean up complete 0 of 2 deleted." | ||
assert len(resp_json["failed_category_ids"]) == 2 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 simplerThere 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.
Yes I think that would be simpler but I will leave it up to you