-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add DutyOfCare Procedure #212
Add DutyOfCare Procedure #212
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #212 +/- ##
==========================================
- Coverage 98.20% 98.20% -0.01%
==========================================
Files 555 562 +7
Lines 12509 12542 +33
Branches 2583 2585 +2
==========================================
+ Hits 12285 12317 +32
- Misses 221 222 +1
Partials 3 3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments below, mostly minor consistency things.
@ray-lee I just push a few commits that I believe resolves everything. I renamed the On the app layer all the field names were simplified and missing attributes are present. In the services layer the xsd was updated for the field names. |
@@ -87,15 +87,11 @@ export default (configContext) => { | |||
}, | |||
}, | |||
}, | |||
dutyOfCareTitle: { | |||
title: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment on the redundancy of "Duty of care title" was really just about the label, since people see it and I want to keep the UI from being too busy. I don't care all that much about the field name, so as far as I'm concerned you didn't need to change it. But it's fine that you did. Kristina may care more about how the fields are named.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I've definitely been a little bit more paranoid about the field names from the qa testing. I didn't really see the need for dutyofcare_commons.dutyofcare*
in the db when revising the fields, but I'll talk w/ Kristina about it as well for the upcoming procedures.
I was planning on asking her for the data qa testing on dev once the procedures go up as well since it might be overwhelming to do the 6 or 7 new procedures we're getting at once.
What does this do?
Why are we doing this? (with JIRA link)
Jira: https://collectionspace.atlassian.net/browse/DRYD-1332
This is one of the procedures for the NAGPRA support we're working on for the next release.
How should this be tested? Do these changes have associated tests?
npm run lint
npm run test
Dependencies for merging? Releasing to production?
I didn't any fields to the advanced search config. I was going to go through the fields and look for good candidates, but I know there's also been talk about reworking that page. Will bring it up in the weekly meeting as it seems like something good to discuss as a group.
Has the application documentation been updated for these changes?
No
Did someone actually run this code to verify it works?
@mikejritter tested against a local install