-
Notifications
You must be signed in to change notification settings - Fork 40
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
DRYD-1474: added term 'partially approved' to deaccessionapprovalstatus #307
DRYD-1474: added term 'partially approved' to deaccessionapprovalstatus #307
Conversation
…us in defaults/base-instance-vocabularies.xml
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.
@axb21 This is good for core but the term needs to be added for other profiles as well.
One of the annoying thing about CollectionSpace is the amount of duplicated configuration everywhere. In this case the deaccessionapprovalstatus
exists in multiple *-vocabularies.xml
files:
defaults/base-instance-vocabularies.xml
defaults/extensions/fcart-profile-instance-vocabularies.xml
tenants/botgarden/base-instance-vocabularies.xml
tenants/materials/base-instance-vocabularies.xml
tenants/publicart/base-instance-vocabularies.xml
Thanks @mikejritter I'll take a look. How do you end up testing that you've gotten the configuration updated in the right places? I was thrown by the name |
@axb21 Testing these changes is also... not great. Normally I'll do a clean build of collectionspace and use curl to check that the term list + terms exist. It's something like:
Recently though I've been looking at the actual result of the build because there's a bunch of intermediary files which are created. This is a little faster and doesn't rely on us needing to enable each profile (I think, at least). In tomcat, the build will write its config to There has been talk in the past about restructuring the base-instance-vocabularies but that predates me so I'm not exactly sure what ideas people had. |
…us in several more vocabularies (fcart, botgarden, material, publicart)
@mikejritter I added partially approved to fcart, botgarden, materials, and publicart as well. I checked that the terms were present in the corresponding |
A sidenote: using <pageNum>0</pageNum>
<pageSize>40</pageSize>
<itemsInPage>40</itemsInPage>
<totalItems>178</totalItems> meaning you're only getting 40 out of 178 items from that call. It turns out the things I added are not in the first page so it cannot be used to verify that the addition worked. Off the top of my head I don't know how to get the pages past page 0 out of this list (it's |
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.
@axb21 looks like the term everywhere now. For the page size and page number, I think collectionspace uses pgSz
and pgNum
for its query parameters.
btw I've been merging into the v8.1-branch
for the moment as it was part of the release instructions. It'd be a good idea to revisit the workflows once the release is done to make it a little less cumbersome but thought I'd let you know.
What does this do?
Update default terms in deaccessionapprovalstatus (Approval Status) term list to include the option "partially approved"
Why are we doing this? (with JIRA link)
Jira ticket: https://collectionspace.atlassian.net/browse/DRYD-1474
How should this be tested? Do these changes have associated tests?
Dependencies for merging? Releasing to production?
None
Has the application documentation been updated for these changes?
No
Did someone actually run this code to verify it works?
@axb21 tested as per above procedure (see screenshot)
**Screenshot
