Closed ghost closed 1 year ago
No. I do not have any plans of making any more changes unless it does not pass acceptance criterium.
I would have merge be defined by acceptance criterium defined by the product owner. In this case, I would suspect that means running and tested in dev by developers followed by running and tested in QA by assigned personnel that are intended application users.
Based on 2022-08-26 conversation: Please factor out all mentions of "harvard.edu" and "dataverse"... as those details will likely not make it through an upstream DSpace PR.
Summary of what I mentioned in today's meeting:
In terms of bringing this back to DSpace, keep in mind our Code Contribution Guidelines...see quick checklist at https://wiki.lyrasis.org/display/DSPACE/Code+Contribution+Guidelines#CodeContributionGuidelines-ContributionChecklist
A few items in that checklist that often trip people up:
More specific to this work (based on the Java code I've seen);
Thanks again for the hard work here!
@tdonohue thanks for the guidance on preparing code for upstream contribution. I will perform a cleanup pass to ensure all JavaDocs are there and adequate, attempt to refactor to ensure clear separation between LDN and COAR Notify, add some documentation to the readme, and if time permits add additional test coverage.
Regarding the dc.data.uri
metadata field, this is what was selected by HDC. We can certainly move it to either a COAR or DSpace schema. Which would you prefer? Or should this be a question for wider community?
Last but not least, I am pretty sure checkstyles pass on all new code.
Thanks
@wwelling : Yes, checkstyle can be run via Maven using mvn checkstyle:check
.
For dc.data.uri
, I'd likely recommend just adding it to the existing dspace.*
schema (dspace.data.uri or similar) unless there's a reason to have multiple coar.*
metadata fields. In other words, there's really no reason to create a new schema for a single metadata field.
@awoods would it make sense to merge this into main-coar-notify
and then pull into main-hdc
before making any further changes for upstream contribution. I feel preparing code for upstream contribution is beyond the scope of this issue. Additionally, I would like to continue the code quality improvements in collaboration with 4science.
Refactor release notification to COAR Notify announce relationship.
Improvement could be made in how to determine which property of the notification holds the DSpace item URL. Before refactoring it was consistent with all notification types from DSpace 6 MVP as being the
context.id
. Now the suggested schema from COAR Notify places it inobject.object
forRelationshipAction
.Following: https://docs.google.com/document/d/1Y-XpG5-Y748LlxORIcMhLP9HdnMVdatN8m03wtAsKyU/edit#
For now, is a redundant bit of logic using
context.id
if present else usingobject.object
. This works as theRelationshipAction
does not providecontext
property.https://github.com/harvard-lts/DSpace/pull/53/files#diff-20b6cf4f382158808b7b8b5b94cea8bd69951f39419e481a4b20a54539f60350R90
https://github.com/harvard-lts/DSpace/pull/53/files#diff-91b93a174de5bc5e80cf6909fb8f8d9fa7fff750e5cd9fe8873754e0650398e7R276
Deferring improved implementation to extract DSpace item URL from notification as it seems like an inconsistency that should be discussed. Additionally,
object.object
does not seem to be an ideal property path. :(