Skip to content

jir-data-visibility-review-feature #298

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.

Sign up for GitHub

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

Merged
jirhiker merged 2 commits into from
Dec 13, 2025
Merged

jir-data-visibility-review-feature #298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f6e94f
feat: update data visibility and review scenarios for clarity and con…
jirhiker Dec 12, 2025
a267088
fix: update validation error status code for new record creation
jirhiker Dec 13, 2025
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
139 changes: 84 additions & 55 deletions tests/features/data-visibility-and-review.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

@backend @BDMS-XXX
Feature: Manage data visibility separately from review and approval in the backend
As an Ocotillo user
As an Ocotillo data manager
I want visibility/privacy and review/approval to be stored as two separate backend attributes
So that the system can independently control who can see the data and which data is reviewed

Expand All @@ -16,78 +16,107 @@ Feature: Manage data visibility separately from review and approval in the backe

Background:
Given a functioning api
And the dataset model has a visibility field that supports "internal" and "public"
And the dataset model has a review_status field that supports "provisional" and "approved"
And visibility and review_status are required fields when creating new datasets
And new datasets must use visibility and review_status as the source of truth
And all database models have a visibility field that supports "internal" and "public"
And all database models have a review_status field that supports "provisional" and "approved"
And visibility and review_status are required fields when creating new records
# hard to test?

@jacob-a-brown jacob-a-brown Dec 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it'll be too hard to test. We can just make sure in BaseCreateSchema that those fields are always required for new records, so if they are not submitted Pydantic will raise errors.

@jirhiker jirhiker Dec 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So for this step you would assert that BaseCreateSchema has these fields and they are required?

@jirhiker jirhiker Dec 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

what test could be used to assert "And legacy combined visibility/review fields are deprecated"

@jacob-a-brown jacob-a-brown Dec 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the step visibility and review_status are required fields when creating new records I would assert that BaseCreateSchema has these fields and that every Create... schema inherits from BaseCreateSchema. Or, I suppose, we could test that every Create... schema has these fields, though it'd be nice to test the former to ensure that all Create... schemas have parity

And new records must use visibility and review_status as the source of truth
And legacy combined visibility/review fields are deprecated

Scenario: Require visibility and review status when creating a new dataset
When the user attempts to create a new dataset via the api without specifying visibility or review_status
Then the system should return a validation error status code (such as 400)

Scenario Outline: Require visibility and review status when creating a new record
Comment thread
jirhiker marked this conversation as resolved.
When I create a new <model> without specifying visibility or review_status
Then the system should return a 422 status code
And the system should not persist the new dataset
And the error response should indicate that visibility is required
And the error response should indicate that review_status is required
And the error response should not include default values for visibility or review_status

Scenario: Update visibility for a new dataset without changing review status
Given the system has a new dataset with visibility "internal" and review_status "provisional"
When the user updates the dataset visibility to "public" via the api
Then the system should persist the dataset with visibility "public"
And the system should preserve the review_status as "provisional"
And retrieving the dataset via the api should return visibility "public" and review_status "provisional"

Scenario: Update review status for a new dataset without changing visibility
Given the system has a new dataset with visibility "internal" and review_status "provisional"
When an authorized reviewer updates the dataset review_status to "approved" via the api
Then the system should persist the dataset with review_status "approved"
And the system should preserve the visibility as "internal"
And retrieving the dataset via the api should return visibility "internal" and review_status "approved"

Scenario: Allow all datasets for internal users
Given the system has datasets with combinations of:
| visibility | review_status |
| internal | provisional |
| internal | approved |
| public | provisional |
| public | approved |
And the caller is identified as an internal staff user
When the internal staff user requests those datasets via the api
Then the system should return all of those datasets in the response

Scenario: Include disclaimer information based on review status
Given the system has a dataset with visibility "public" and review_status "provisional"
When any authorized caller retrieves the dataset via the api
Then the system should return a response in JSON format
And the response should include a disclaimer field derived from the review_status
And the disclaimer should indicate that the data is provisional
# Given Require visibility and review status when creating a new record. no default for visibility and review
# status will be defined so this is impossible to test
And the error response should not include default values for visibility or review_status

And the system has a dataset with visibility "internal" and review_status "approved"
When any authorized caller retrieves that dataset via the api
Then the response should include a disclaimer field derived from the review_status
And the disclaimer should indicate that the data is approved
Examples:
|model|
|Well |
|Contact|
|Observation|

Scenario: Migrate legacy combined visibility/review values to separate fields
Given the system has legacy records with a combined visibility/review field
Scenario Outline: Migrate legacy combined visibility/review values to separate fields
Given the system has legacy records with a combined visibility/review field for <legacy_table>
And there is a documented mapping from each legacy value to a visibility and review_status pair
When the migration job runs to populate visibility and review_status for legacy records
When the migration job runs to populate visibility and review_status for <legacy_table> records
Then the system should set visibility according to the documented mapping
And the system should set review_status according to the documented mapping
# hard to test?

@jacob-a-brown jacob-a-brown Dec 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can put these mappings into a test and then ensure that the function works correctly in some unit tests

@jirhiker jirhiker Dec 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hard to test? refers to "the system should stop using the legacy combined field as the source of truth
And subsequent updates to migrated records should modify visibility and review_status fields only". How would you test for that

@jacob-a-brown jacob-a-brown Dec 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Off the top of my head we can go through every model in the database and ensure that none of the legacy fields exist and all that is there are visibility and review_status

And the system should stop using the legacy combined field as the source of truth
And subsequent updates to migrated records should modify visibility and review_status fields only

Examples:
|legacy_table|
|Location |
|WaterLevels |
|WaterLevelsContinuous_Acoustic|
|WaterLevelsContinuous_Pressure|

Scenario: Spot-check migrated legacy records against the documented mapping
Given the migration job has completed
And the tester selects a sample of migrated records with known legacy values
When the tester retrieves each sampled record via the api
Then the visibility value for each sampled record should match the expected mapped visibility
And the review_status value for each sampled record should match the expected mapped review_status
And no sampled record should expose or rely on the legacy combined field for visibility or review behavior

Scenario: Reject unauthorized changes to visibility or review status
Given the system has a dataset with visibility "internal" and review_status "provisional"
And the caller is authenticated without permission to change visibility or review_status
When the caller attempts to update the visibility to "public" via the api
And the caller attempts to update the review_status to "approved" via the api
Then the system should return an authorization error for these updates
And the dataset should remain persisted with visibility "internal" and review_status "provisional"
#
# @skip
# @patch
# Scenario: Update visibility for a new dataset without changing review status
# Given the system has a new dataset with visibility "internal" and review_status "provisional"
# When the user updates the dataset visibility to "public" via the api
# Then the system should persist the dataset with visibility "public"
# And the system should preserve the review_status as "provisional"
# And retrieving the dataset via the api should return visibility "public" and review_status "provisional"
#
# @skip
# @patch
# Scenario: Update review status for a new dataset without changing visibility
# Given the system has a new dataset with visibility "internal" and review_status "provisional"
# When an authorized reviewer updates the dataset review_status to "approved" via the api
# Then the system should persist the dataset with review_status "approved"
# And the system should preserve the visibility as "internal"
# And retrieving the dataset via the api should return visibility "internal" and review_status "approved"
#
# @skip
# @authorization
# Scenario: Allow all datasets for internal users
# Given the system has datasets with combinations of:
# | visibility | review_status |
# | internal | provisional |
# | internal | approved |
# | public | provisional |
# | public | approved |
# And the caller is identified as an internal staff user
# When the internal staff user requests those datasets via the api
# Then the system should return all of those datasets in the response
#
# @skip @authorization
# Scenario: Reject unauthorized changes to visibility or review status
# Given the system has a dataset with visibility "internal" and review_status "provisional"
# And the caller is authenticated without permission to change visibility or review_status
# When the caller attempts to update the visibility to "public" via the api
# And the caller attempts to update the review_status to "approved" via the api
# Then the system should return an authorization error for these updates
# And the dataset should remain persisted with visibility "internal" and review_status "provisional"
#
# @skip
# @disclaimer
# Scenario: Include disclaimer information based on review status
# Given the system has a dataset with visibility "public" and review_status "provisional"
# When any authorized caller retrieves the dataset via the api
# Then the system should return a response in JSON format
# And the response should include a disclaimer field derived from the review_status
# And the disclaimer should indicate that the data is provisional
#
# And the system has a dataset with visibility "internal" and review_status "approved"
# When any authorized caller retrieves that dataset via the api
# Then the response should include a disclaimer field derived from the review_status
# And the disclaimer should indicate that the data is approved
#
Loading
Loading