From 15a01a2813afbaa15c87d7aae77cf884afa8db24 Mon Sep 17 00:00:00 2001 From: Omar Abou Selo Date: Mon, 23 Sep 2024 15:04:28 +0300 Subject: [PATCH] Environment issues followup (#217) * Add is_confirmed query parameter to environment issues endpoint * Make environment issue URL optional if issue is unconfirmed * Make environment issues URL optional for unconfirmed cases in frontend --- ...43_make_environment_issues_url_nullable.py | 27 ++++++++++++ .../controllers/environments/models.py | 21 ++++----- .../environments/reported_issues.py | 9 +++- backend/test_observer/data_access/models.py | 3 +- .../environments/test_environment_issues.py | 43 ++++++++++++++++++- frontend/lib/models/environment_issue.dart | 2 +- frontend/lib/repositories/api_repository.dart | 2 +- .../environment_issue_form.dart | 12 +++++- .../environment_issue_list_item.dart | 12 +++--- 9 files changed, 108 insertions(+), 23 deletions(-) create mode 100644 backend/migrations/versions/2024_09_23_0840-9d7eed94a543_make_environment_issues_url_nullable.py diff --git a/backend/migrations/versions/2024_09_23_0840-9d7eed94a543_make_environment_issues_url_nullable.py b/backend/migrations/versions/2024_09_23_0840-9d7eed94a543_make_environment_issues_url_nullable.py new file mode 100644 index 00000000..8b169bac --- /dev/null +++ b/backend/migrations/versions/2024_09_23_0840-9d7eed94a543_make_environment_issues_url_nullable.py @@ -0,0 +1,27 @@ +"""Make environment issues URL nullable + +Revision ID: 9d7eed94a543 +Revises: 505b96fd7731 +Create Date: 2024-09-23 08:40:44.972779+00:00 + +""" +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "9d7eed94a543" +down_revision = "505b96fd7731" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.alter_column( + "environment_issue", "url", existing_type=sa.VARCHAR(), nullable=True + ) + + +def downgrade() -> None: + op.alter_column( + "environment_issue", "url", existing_type=sa.VARCHAR(), nullable=False + ) diff --git a/backend/test_observer/controllers/environments/models.py b/backend/test_observer/controllers/environments/models.py index 631a4feb..05145ad9 100644 --- a/backend/test_observer/controllers/environments/models.py +++ b/backend/test_observer/controllers/environments/models.py @@ -1,6 +1,6 @@ from datetime import datetime -from pydantic import BaseModel, HttpUrl, field_validator +from pydantic import BaseModel, HttpUrl, model_validator from test_observer.common.constants import VALID_ISSUE_HOSTS @@ -8,24 +8,25 @@ class EnvironmentReportedIssueRequest(BaseModel): environment_name: str description: str - url: HttpUrl + url: HttpUrl | None = None is_confirmed: bool - @field_validator("url") - @classmethod - def url_host_must_be_allowed( - cls: type["EnvironmentReportedIssueRequest"], url: HttpUrl - ) -> HttpUrl: - if url.host not in VALID_ISSUE_HOSTS: + @model_validator(mode="after") + def validate_url(self) -> "EnvironmentReportedIssueRequest": + if self.url is None and self.is_confirmed: + raise ValueError("A URL is required if the issue is confirmed") + + if self.url is not None and self.url.host not in VALID_ISSUE_HOSTS: raise ValueError(f"Issue url must belong to one of {VALID_ISSUE_HOSTS}") - return url + + return self class EnvironmentReportedIssueResponse(BaseModel): id: int environment_name: str description: str - url: HttpUrl + url: HttpUrl | None is_confirmed: bool created_at: datetime updated_at: datetime diff --git a/backend/test_observer/controllers/environments/reported_issues.py b/backend/test_observer/controllers/environments/reported_issues.py index c56a2e28..fd83f357 100644 --- a/backend/test_observer/controllers/environments/reported_issues.py +++ b/backend/test_observer/controllers/environments/reported_issues.py @@ -13,8 +13,13 @@ @router.get(endpoint, response_model=list[EnvironmentReportedIssueResponse]) -def get_reported_issues(db: Session = Depends(get_db)): - return db.execute(select(EnvironmentIssue)).scalars() +def get_reported_issues( + is_confirmed: bool | None = None, db: Session = Depends(get_db) +): + stmt = select(EnvironmentIssue) + if is_confirmed is not None: + stmt = stmt.where(EnvironmentIssue.is_confirmed == is_confirmed) + return db.execute(stmt).scalars() @router.post(endpoint, response_model=EnvironmentReportedIssueResponse) diff --git a/backend/test_observer/data_access/models.py b/backend/test_observer/data_access/models.py index ab805c82..b772b1a0 100644 --- a/backend/test_observer/data_access/models.py +++ b/backend/test_observer/data_access/models.py @@ -488,7 +488,7 @@ class EnvironmentIssue(Base): __tablename__ = "environment_issue" environment_name: Mapped[str] - url: Mapped[str] + url: Mapped[str | None] = mapped_column(default=None) description: Mapped[str] is_confirmed: Mapped[bool] @@ -498,4 +498,5 @@ def __repr__(self) -> str: "environment_name", "url", "description", + "is_confirmed", ) diff --git a/backend/tests/controllers/environments/test_environment_issues.py b/backend/tests/controllers/environments/test_environment_issues.py index 7a37d5b4..c502e646 100644 --- a/backend/tests/controllers/environments/test_environment_issues.py +++ b/backend/tests/controllers/environments/test_environment_issues.py @@ -20,7 +20,7 @@ def test_empty_get(test_client: TestClient): @pytest.mark.parametrize( "field", - ["url", "description", "environment_name", "is_confirmed"], + ["description", "environment_name", "is_confirmed"], ) def test_post_requires_field(test_client: TestClient, field: str): data = {k: v for k, v in valid_post_data.items() if k != field} @@ -28,6 +28,25 @@ def test_post_requires_field(test_client: TestClient, field: str): assert_fails_validation(response, field, "missing") +def test_url_is_required_if_confirmed(test_client: TestClient): + data = {**valid_post_data, "url": None} + + response = test_client.post(endpoint, json=data) + + assert response.status_code == 422 + + +def test_url_not_required_if_unconfirmed(test_client: TestClient): + data = {**valid_post_data, "is_confirmed": False} + data.pop("url") + + response = test_client.post(endpoint, json=data) + json = response.json() + + assert response.status_code == 200 + _assert_reported_issue(json, json) + + def test_post_validates_url(test_client: TestClient): data = {**valid_post_data, "url": "invalid url"} response = test_client.post(endpoint, json=data) @@ -68,6 +87,28 @@ def test_post_three_then_get(test_client: TestClient): _assert_reported_issue(json[2], issue3) +def test_get_needs_confirmation(test_client: TestClient): + confirmed_issue = { + **valid_post_data, + "description": "Confirmed", + "is_confirmed": True, + } + unconfirmed_issue = { + **valid_post_data, + "description": "Unconfirmed", + "is_confirmed": False, + } + + test_client.post(endpoint, json=confirmed_issue) + test_client.post(endpoint, json=unconfirmed_issue) + + response = test_client.get(endpoint, params={"is_confirmed": False}) + assert response.status_code == 200 + json = response.json() + assert len(json) == 1 + _assert_reported_issue(json[0], unconfirmed_issue) + + def test_update_description(test_client: TestClient): response = test_client.post(endpoint, json=valid_post_data) issue = response.json() diff --git a/frontend/lib/models/environment_issue.dart b/frontend/lib/models/environment_issue.dart index 51d546a9..04fcf5fe 100644 --- a/frontend/lib/models/environment_issue.dart +++ b/frontend/lib/models/environment_issue.dart @@ -9,7 +9,7 @@ class EnvironmentIssue with _$EnvironmentIssue { required int id, @JsonKey(name: 'environment_name') required String environmentName, required String description, - required String url, + required String? url, @JsonKey(name: 'is_confirmed') required bool isConfirmed, }) = _EnvironmentIssue; diff --git a/frontend/lib/repositories/api_repository.dart b/frontend/lib/repositories/api_repository.dart index 4b2095c0..03fc5445 100644 --- a/frontend/lib/repositories/api_repository.dart +++ b/frontend/lib/repositories/api_repository.dart @@ -156,7 +156,7 @@ class ApiRepository { final response = await dio.post( '/v1/environments/reported-issues', data: { - 'url': url, + 'url': url.isEmpty ? null : url, 'description': description, 'environment_name': environmentName, 'is_confirmed': isConfirmed, diff --git a/frontend/lib/ui/artefact_page/environment_issues/environment_issue_form.dart b/frontend/lib/ui/artefact_page/environment_issues/environment_issue_form.dart index a9dc374b..f6e9f021 100644 --- a/frontend/lib/ui/artefact_page/environment_issues/environment_issue_form.dart +++ b/frontend/lib/ui/artefact_page/environment_issues/environment_issue_form.dart @@ -45,7 +45,7 @@ class _EnvironmentIssueUpdateForm extends ConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { return _EnvironmentIssueForm( - initialUrl: issue.url, + initialUrl: issue.url ?? '', initialDescription: issue.description, initialIsConfirmed: issue.isConfirmed, formSubtitle: 'On all environments with name: ${issue.environmentName}', @@ -151,7 +151,15 @@ class _EnvironmentIssueFormState extends ConsumerState<_EnvironmentIssueForm> { VanillaTextInput( label: 'Url', controller: _urlController, - validator: validateIssueUrl, + validator: (url) { + if ((url == null || url.isEmpty) && _isConfirmed) { + return 'Confirmed environment issues must have a URL'; + } + + if (url != null && url.isNotEmpty) return validateIssueUrl(url); + + return null; + }, ), const SizedBox(height: Spacing.level3), VanillaTextInput( diff --git a/frontend/lib/ui/artefact_page/environment_issues/environment_issue_list_item.dart b/frontend/lib/ui/artefact_page/environment_issues/environment_issue_list_item.dart index 775b41e9..cb9ad8c7 100644 --- a/frontend/lib/ui/artefact_page/environment_issues/environment_issue_list_item.dart +++ b/frontend/lib/ui/artefact_page/environment_issues/environment_issue_list_item.dart @@ -12,6 +12,7 @@ class EnvironmentIssueListItem extends StatelessWidget { @override Widget build(BuildContext context) { + final issueUrl = issue.url; return ListTile( title: Tooltip( message: issue.description, @@ -36,11 +37,12 @@ class EnvironmentIssueListItem extends StatelessWidget { ?.copyWith(color: Colors.yellow.shade800), ), const SizedBox(width: Spacing.level4), - InlineUrlText( - url: issue.url, - urlText: 'URL', - fontStyle: Theme.of(context).textTheme.bodyMedium, - ), + if (issueUrl != null) + InlineUrlText( + url: issueUrl, + urlText: 'URL', + fontStyle: Theme.of(context).textTheme.bodyMedium, + ), TextButton( onPressed: () => showEnvironmentIssueUpdateDialog( context: context,