jir-data-visibility-review-feature#298
Conversation
jirhiker
changed the base branch from
staging
to
data-visibility-review-feature
Codecov Report✅ All modified and coverable lines are covered by tests. |
chasetmartin
left a comment
chasetmartin
left a comment
There was a problem hiding this comment.
I like that this keeps some of the scenarios for potential future work as comments, but simplifies the feature for work now.
One question: is it challenging to test "according to the documented mapping" - or is that just a matter of having the mapping referenced somewhere in the code implementation?
That's what I was thinking - I'm not sure what exactly the mapping will actually be at this moment but I figured Kelsey/Jacob would be able to implement that when they do the mapping |
| 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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So for this step you would assert that BaseCreateSchema has these fields and they are required?
There was a problem hiding this comment.
what test could be used to assert "And legacy combined visibility/review fields are deprecated"
There was a problem hiding this comment.
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
| 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? |
There was a problem hiding this comment.
We can put these mappings into a test and then ensure that the function works correctly in some unit tests
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
5 participants
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?