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

feat: Remove inbound sms from service #4312

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion app/dao/inbound_numbers_dao.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from uuid import UUID

from app import db
from app.dao.dao_utils import autocommit
from app.constants import INBOUND_SMS_TYPE
from app.dao.dao_utils import autocommit, transaction
from app.models import InboundNumber


Expand Down Expand Up @@ -41,3 +44,30 @@ def dao_allocate_number_for_service(service_id, inbound_number_id):
if not updated:
raise Exception(f"Inbound number: {inbound_number_id} is not available")
return InboundNumber.query.get(inbound_number_id)


def archive_or_release_inbound_number_for_service(service_id: UUID, archive: bool, commit=True):
update_data = {
"service_id": None,
}

if archive:
update_data["active"] = False

result = InboundNumber.query.filter_by(service_id=service_id, active=True).update(
update_data, synchronize_session="fetch"
)

if commit:
db.session.commit()
return result


def dao_remove_inbound_sms_for_service(service_id, archive):
from app.dao.service_permissions_dao import dao_remove_service_permission
from app.dao.service_sms_sender_dao import dao_remove_inbound_sms_senders

with transaction():
dao_remove_service_permission(service_id, INBOUND_SMS_TYPE, commit=False)
dao_remove_inbound_sms_senders(service_id, commit=False)
archive_or_release_inbound_number_for_service(service_id, archive, commit=False)
48 changes: 48 additions & 0 deletions app/dao/inbound_sms_dao.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
from datetime import datetime
from uuid import UUID

from flask import current_app
from sqlalchemy import and_, desc
from sqlalchemy.dialects.postgresql import insert
Expand All @@ -7,8 +10,10 @@
from app.constants import SMS_TYPE
from app.dao.dao_utils import autocommit
from app.models import (
InboundNumber,
InboundSms,
InboundSmsHistory,
Notification,
Service,
ServiceDataRetention,
)
Expand Down Expand Up @@ -179,3 +184,46 @@ def dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(service
)

return q.paginate(page=page, per_page=current_app.config["PAGE_SIZE"])


def dao_get_most_recent_inbound_usage_date(service_id: UUID, inbound: InboundNumber) -> datetime | None:
last_notification = (
Notification.query.filter(
Notification.reply_to_text == inbound.number,
Notification.service_id == service_id,
)
.order_by(Notification.created_at.desc())
.first()
)

last_inbound_sms = (
InboundSms.query.filter(
InboundSms.notify_number == inbound.number,
InboundSms.service_id == service_id,
)
.order_by(InboundSms.created_at.desc())
.first()
)

last_inbound_sms_history = (
InboundSmsHistory.query.filter(
InboundSmsHistory.notify_number == inbound.number,
InboundSmsHistory.service_id == service_id,
)
.order_by(InboundSmsHistory.created_at.desc())
.first()
)

most_recent_usage = max(
filter(
None,
[
last_notification.created_at if last_notification else None,
last_inbound_sms.created_at if last_inbound_sms else None,
last_inbound_sms_history.created_at if last_inbound_sms_history else None,
],
),
default=None,
)

return most_recent_usage
10 changes: 6 additions & 4 deletions app/dao/service_permissions_dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ def dao_add_service_permission(service_id, permission):
db.session.add(service_permission)


def dao_remove_service_permission(service_id, permission):
deleted = ServicePermission.query.filter(
def dao_remove_service_permission(service_id, permission, commit=True):
result = ServicePermission.query.filter(
ServicePermission.service_id == service_id, ServicePermission.permission == permission
).delete()
db.session.commit()
return deleted

if commit:
db.session.commit()
return result
14 changes: 14 additions & 0 deletions app/dao/service_sms_sender_dao.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from uuid import UUID

from sqlalchemy import desc

from app import db
Expand Down Expand Up @@ -106,3 +108,15 @@ def _raise_when_no_default(old_default):
# check that the update is not updating the only default to false
if not old_default:
raise Exception("You must have at least one SMS sender as the default.", 400)


def dao_remove_inbound_sms_senders(service_id: UUID, commit=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def dao_remove_inbound_sms_senders(service_id: UUID, commit=True):
@autocommit
def dao_remove_inbound_sms_senders(service_id: UUID):
result = (
ServiceSmsSender.query.filter_by(service_id=service_id)
.filter(ServiceSmsSender.inbound_number_id.isnot(None))
.delete(synchronize_session="fetch")
)
return result

I am not sure but is commit=True and flag necessary? transaction is a generator function and it handles the sessions and commits.

result = (
ServiceSmsSender.query.filter_by(service_id=service_id)
.filter(ServiceSmsSender.inbound_number_id.isnot(None))
.delete(synchronize_session="fetch")
)

if commit:
db.session.commit()
return result
14 changes: 14 additions & 0 deletions app/inbound_sms/inbound_sms_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,17 @@
"phone_number": {"type": "string"},
},
}

remove_inbound_sms_for_service_schema = {
"$schema": "http://json-schema.org/draft-07/schema#",
"description": "Schema for validating the input to remove inbound SMS capability",
"type": "object",
"properties": {
"archive": {
"type": "boolean",
"description": "Indicates whether to archive the inbound number or release it.",
},
},
"required": ["archive"],
"additionalProperties": False,
}
40 changes: 38 additions & 2 deletions app/inbound_sms/rest.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
from flask import Blueprint, jsonify, request
from flask import Blueprint, current_app, jsonify, request
from notifications_utils.recipient_validation.phone_number import try_validate_and_format_phone_number

from app.dao.inbound_numbers_dao import (
dao_get_inbound_number_for_service,
dao_remove_inbound_sms_for_service,
)
from app.dao.inbound_sms_dao import (
dao_count_inbound_sms_for_service,
dao_get_inbound_sms_by_id,
dao_get_inbound_sms_for_service,
dao_get_most_recent_inbound_usage_date,
dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service,
)
from app.dao.service_data_retention_dao import fetch_service_data_retention_by_notification_type
from app.errors import register_errors
from app.inbound_sms.inbound_sms_schemas import get_inbound_sms_for_service_schema
from app.inbound_sms.inbound_sms_schemas import (
get_inbound_sms_for_service_schema,
remove_inbound_sms_for_service_schema,
)
from app.schema_validation import validate

inbound_sms = Blueprint("inbound_sms", __name__, url_prefix="/service/<uuid:service_id>/inbound-sms")
Expand Down Expand Up @@ -60,3 +68,31 @@ def get_inbound_by_id(service_id, inbound_sms_id):
message = dao_get_inbound_sms_by_id(service_id, inbound_sms_id)

return jsonify(message.serialize()), 200


@inbound_sms.route("/remove", methods=["POST"])
def remove_inbound_sms_for_service(service_id):
data = request.get_json()
validate(data, remove_inbound_sms_for_service_schema)

archive = data["archive"]

try:
dao_remove_inbound_sms_for_service(service_id, archive)
return jsonify({}), 200

except Exception as e:
current_app.logger.error("error removing inbound SMS for service %s: %s", service_id, e)
return jsonify({"message": str(e)}), 500


@inbound_sms.route("/most-recent-usage", methods=["GET"])
def get_most_recent_inbound_usage_date(service_id):
inbound = dao_get_inbound_number_for_service(service_id)

if not inbound:
return jsonify(message="inbound not found"), 404

most_recent_date = dao_get_most_recent_inbound_usage_date(service_id, inbound)

return jsonify(most_recent_date=most_recent_date), 200
17 changes: 17 additions & 0 deletions tests/app/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from app.history_meta import create_history
from app.models import (
ApiKey,
InboundSmsHistory,
InvitedUser,
Job,
Notification,
Expand Down Expand Up @@ -1043,6 +1044,22 @@ def sample_inbound_numbers(sample_service):
return inbound_numbers


@pytest.fixture
def sample_inbound_sms_history(notify_db_session, sample_service):
inbound_sms = InboundSmsHistory(
id=uuid.uuid4(),
created_at=datetime.utcnow(),
service_id=sample_service.id,
notify_number="3",
provider_date=datetime.utcnow(),
provider_reference="reference-id",
provider="firetext",
)
notify_db_session.add(inbound_sms)
notify_db_session.commit()
return inbound_sms


@pytest.fixture
def sample_organisation(notify_db_session):
org = Organisation(name="sample organisation")
Expand Down
91 changes: 90 additions & 1 deletion tests/app/dao/test_inbound_numbers_dao.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
import pytest
from sqlalchemy.exc import IntegrityError
from sqlalchemy.exc import IntegrityError, SQLAlchemyError

from app.constants import INBOUND_SMS_TYPE
from app.dao.inbound_numbers_dao import (
archive_or_release_inbound_number_for_service,
dao_allocate_number_for_service,
dao_get_available_inbound_numbers,
dao_get_inbound_number,
dao_get_inbound_number_for_service,
dao_get_inbound_numbers,
dao_remove_inbound_sms_for_service,
dao_set_inbound_number_active_flag,
dao_set_inbound_number_to_service,
)
from app.dao.service_sms_sender_dao import dao_add_sms_sender_for_service, dao_get_sms_senders_by_service_id
from app.models import InboundNumber
from tests.app.db import create_inbound_number, create_service

Expand Down Expand Up @@ -104,3 +109,87 @@ def test_dao_allocate_number_for_service_raises_if_invalid_inbound_number(notify
with pytest.raises(Exception) as exc:
dao_allocate_number_for_service(service_id=service.id, inbound_number_id=fake_uuid)
assert "is not available" in str(exc.value)


def test_archive_or_release_inbound_number_for_service_archive(sample_service, sample_inbound_numbers):
inbound = next((inbound for inbound in sample_inbound_numbers if inbound.service_id == sample_service.id), None)

archive_or_release_inbound_number_for_service(sample_service.id, True)

updated_inbound = InboundNumber.query.filter_by(number=inbound.number).one_or_none()

assert updated_inbound.service_id is None
assert updated_inbound.active is False


def test_archive_or_release_inbound_number_for_service_release(sample_service, sample_inbound_numbers):
inbound = next((inbound for inbound in sample_inbound_numbers if inbound.service_id == sample_service.id), None)

archive_or_release_inbound_number_for_service(sample_service.id, False)

updated_inbound = InboundNumber.query.filter_by(number=inbound.number).one_or_none()

assert updated_inbound.service_id is None
assert updated_inbound.active is True


@pytest.mark.parametrize(
"archive, inbound_number, expected_active_status",
[
(True, "7654321", False),
(False, "1234567", True),
],
)
def test_dao_remove_inbound_sms_for_service_success(
admin_request, sample_service_full_permissions, archive, inbound_number, expected_active_status
):
service = sample_service_full_permissions
service_inbound = dao_get_inbound_number_for_service(service.id)
dao_add_sms_sender_for_service(service.id, inbound_number, is_default=True, inbound_number_id=service_inbound.id)
sms_senders = dao_get_sms_senders_by_service_id(service.id)

assert (service.has_permission(INBOUND_SMS_TYPE)) is True
assert any(x.inbound_number_id is not None and x.sms_sender == inbound_number for x in sms_senders) is True
assert service_inbound.active is True
assert service_inbound.service_id is not None

dao_remove_inbound_sms_for_service(service.id, archive=archive)

sms_senders = dao_get_sms_senders_by_service_id(service.id)
updated_service_inbound = dao_get_inbound_number_for_service(service.id)
inbound = dao_get_inbound_number(service_inbound.id)

assert (service.has_permission(INBOUND_SMS_TYPE)) is False
assert any(x.inbound_number_id is not None and x.sms_sender == inbound_number for x in sms_senders) is False
assert updated_service_inbound is None
assert inbound.service_id is None
assert inbound.active is expected_active_status


def test_dao_remove_inbound_sms_for_service_failure(sample_service_full_permissions, mocker, notify_db_session):
inbound_number = "76543953521"
service = sample_service_full_permissions
service_inbound = dao_get_inbound_number_for_service(service.id)
dao_add_sms_sender_for_service(service.id, inbound_number, is_default=True, inbound_number_id=service_inbound.id)

sms_senders = dao_get_sms_senders_by_service_id(service.id)
assert service.has_permission(INBOUND_SMS_TYPE) is True
assert any(x.inbound_number_id is not None and x.sms_sender == inbound_number for x in sms_senders) is True
assert service_inbound.active is True
assert service_inbound.service_id is not None

with mocker.patch(
"app.dao.inbound_numbers_dao.archive_or_release_inbound_number_for_service", side_effect=SQLAlchemyError
):
with pytest.raises(SQLAlchemyError):
dao_remove_inbound_sms_for_service(service.id, archive=True)

sms_senders_after = dao_get_sms_senders_by_service_id(service.id)
updated_service_inbound = dao_get_inbound_number_for_service(service.id)
inbound = dao_get_inbound_number(service_inbound.id)

assert service.has_permission(INBOUND_SMS_TYPE) is True
assert any(x.inbound_number_id is not None and x.sms_sender == inbound_number for x in sms_senders_after) is True
assert updated_service_inbound is not None
assert inbound.service_id == service.id
assert inbound.active is True
Loading