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

[DT-97]Update DAR schema to allow Progress Report entities #2449

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

rjohanek
Copy link
Contributor

Addresses

Ticket:https://broadworkbench.atlassian.net/browse/DT-97

Summary

Add missing fields that represent progress report entities to the DataAccessRequest schema. These fields already exist in the backend in DataAccessRequestData.
This work just coverts the models into yaml schemas and adds them to DataAccessRequest.yaml


Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rjohanek rjohanek requested a review from a team as a code owner January 16, 2025 18:48
@rjohanek rjohanek requested review from rushtong and fboulnois and removed request for a team January 16, 2025 18:48
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

The intent of the original ticket was to also be able to submit these entities, although the acceptance criteria is not explicit about that piece. We could add that to a separate ticket if you'd like, or we could make it part of this ticket.

I think there may need to be some code added to our submit data access request endpoint to support this new functionality.

We should also add some unit tests for the API submission of a progress report if they don't already exist.

@rjohanek
Copy link
Contributor Author

The intent of the original ticket was to also be able to submit these entities, although the acceptance criteria is not explicit about that piece. We could add that to a separate ticket if you'd like, or we could make it part of this ticket.

I think there may need to be some code added to our submit data access request endpoint to support this new functionality.

We should also add some unit tests for the API submission of a progress report if they don't already exist.

Oh okay, yeah the ticket just said to update the schema. Maybe we could talk more about the original intent on Tuesday and when I understand more I can either update this work or make another ticket

Comment on lines 222 to 224
dataManagementIncident:
$ref: './DataManagementIncident.yaml'
description: A Data Management Incident.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a warning here that is a little confusing at first. The root cause is that if there is a $ref, then anything after it, i.e. description, is ignored. The idea is that all information should in theory be inside the $ref, not here in the property. Same thing applies to closeOutSupplement

Copy link
Contributor

Choose a reason for hiding this comment

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

Example of an incorrect intellij warning:

Screenshot 2025-01-21 at 7 48 07 AM

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

This looks correct to me after manually comparing against the existing java classes. See minor comment inline. I'd be happy with this moving forward and a separate PR for ensuring that the existing DAR crud operations work with these objects as defined here 👍🏽

@rjohanek rjohanek requested a review from fboulnois January 21, 2025 14:43
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

👍

@rjohanek rjohanek merged commit 06827e2 into develop Jan 21, 2025
14 checks passed
@rjohanek rjohanek deleted the rj/dt-97-update-DAR-schema branch January 21, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants