Rename properties and support for name variants #2488

benbosman · 2019-08-13T14:11:49Z

This PR contains the implementation for DSpace/RestContract#67

The leftLabel is renamed to leftWardType at the relationship type level to clarify e.g. isAuthorOfPublication is the name of the arrow pointing from the right item to the left item. The term leftLabel created some confusion in the past.
Support for name variants has also been included with an leftWardValue property at the relationship level to specify an alternative name used in the arrow pointing from the right item to the left item.

The name variants can be exposed in the virtual metadata as well, so this direct property will primarily be used for storing and updating the label.

Modified the Relationship so that it also has a leftWardLabel and a rightWardLabel for storing alternative labels (name variants). Also created an sql migration to add two columns for these labels to the relationship table.

Refactor the integration tests for the RelationshipRestRepository to reduce the amount of code duplication. Many tests contained code to create a number of variables to be used in the test, this code was pretty much the same for every test. I refactored the integration test class so that all variables used in multiple tests are now declared at the top and are initialized in an @before function. Test-specific variables are unchanged.

into feature-name-variants

Renamed the columns 'left_label' and 'right_label' in the table 'relationship_type' to 'leftward_label' and 'rightward_label'. Also renamed all instances of [l|L]eftLabel and [r|R]ightLabel to [l|L]eftwardLabel and [r|R]ightwardLabel, respectively.

into feature-name-variants # Conflicts: # dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java # dspace-api/src/main/java/org/dspace/content/RelationshipMetadataServiceImpl.java

Renamed the leftwardLabel and rightwardlabel for Relationship to leftwardValue and rightwardValue. Renamed the same values for RelationshipType to leftwardType and rightwardType.

tdonohue · 2019-08-26T16:23:06Z

@benbosman : Gave this a quick look today. Unfortunately, though this PR is pointing out some more possible confusion in our naming of relationships.

It definitely looks odd to me that we have a RelationshipType object that has properties leftType and rightType (which are both IDs referring to an EntityType) and properties leftwardType and rightwardType (which are both Strings which name the RelationshipType based on the direction of the relationship). This causes some confusingly named methods in this implementation like EntityService.getRelationshipTypesByType...which is using String types (leftwardType and rightwardType) and not EntityTypes (leftType and rightType).

While I like the idea of leftwardType and rightwardType in the REST Contract, I wonder if we need to cleanup the language in the Java Code to be something like leftwardTypeName? Or maybe leftType should really be leftEntityType (to distinguish it as an EntityType and not a RelationshipType)? "Type" itself is just being used in multiple ways, so we may need to clarify which Type we are talking about in the Java/database layer.

Conflicts: dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java

tdonohue

@benbosman : I think this PR needs more attention in the code itself. Each time I try to review it, the method names trip me up (they are very confusing to me), and I end up having to (literally) open up the entire source code to understand what the methods names are trying to say. This implies to me that these methods are likely misnamed (or maybe trying to do too much?), and it'd be extremely hard for other developers to learn.

The best example here is the large number of methods that all start with handle. While it's accurate that they are all "handling" some process, the word handle itself provides no description of what they are doing. Some handle methods seem to be doing validation (like a validate method would), while others seem like they are gathering together information (like a find or get method would). But, overall, the name of the method doesn't give me any clues as to what it is trying to achieve.

Sorry to pester yet again on a PR that seems like it should be "straightforward", but I cannot seem to even get 1/2 way through this PR in over an hour's review time. It's just not making sense to me, without having to try and open up the entire source code :(

Could you take a closer look at the methods names here and see if they make sense to you? Perhaps we can simply clean up the method names to be more descriptive?

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java

dspace-api/src/main/java/org/dspace/content/RelationshipMetadataServiceImpl.java

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java

Add javadocs to complex private methods

benbosman · 2019-09-05T08:08:00Z

@tdonohue @mwoodiupui The method names and JavaDoc has been updated to make it easier to understand.

tdonohue

Thanks for the explanations & updates to the method names @benbosman ! I think the code is looking a ton better. I've noticed on other method with an awkward name handle* (which might be nice to cleanup here). But, more importantly, it looks like we are missing Unit tests in one area (see below). At a minimum, I think the latter should be fixed before merger.

Overall though, nice work. This is looking much better.

dspace-api/src/main/java/org/dspace/content/RelationshipMetadataServiceImpl.java

tdonohue · 2019-09-06T19:42:42Z

dspace-api/src/test/java/org/dspace/content/RelationshipMetadataServiceTest.java

+
+    @Test
+    @Ignore
+    public void testGetRelationshipMetadata() throws Exception {


This test class is essentially empty (unfinished). It seems like we should be implementing tests for the new RelationshipMetadataService interface, and it's single method (getRelationshipMetadata()). There's a lot of complexity in our implementation (RelationshipMetadataServiceImpl) that isn't being tested out via Unit Tests

I've asked Raf to implement the tests for the new RelationshipMetadataService interface.
It's indeed currently only tested indirectly in the ITs, but an explicit test of the service was planned to be included as well

Raf has completed the tests for the new RelationshipMetadataService interface.

dimitrispie

No issues found

paulo-graca

It's working as expected. I just have some minor comments. But overall it's ok by me.

dspace-api/src/main/java/org/dspace/content/RelationshipMetadataServiceImpl.java

dspace-api/src/main/java/org/dspace/content/virtual/UUIDValue.java

dspace-api/src/main/java/org/dspace/content/virtual/VirtualMetadataConfiguration.java

dspace-api/src/main/java/org/dspace/content/virtual/Related.java

dspace-api/src/main/java/org/dspace/content/virtual/Concatenate.java

dspace-api/src/main/java/org/dspace/content/virtual/Collected.java

tdonohue

Overall, I'm nearly a +1 now. I found just a few more minor things to clean up, and then I think this can be merged.

dspace-api/src/main/java/org/dspace/content/RelationshipMetadataService.java

dspace-api/src/test/java/org/dspace/content/RelationshipMetadataServiceTest.java

dspace-server-webapp/src/test/java/org/dspace/app/rest/RelationshipRestRepositoryIT.java

paulo-graca

Thank you @benbosman !

tdonohue

👍 Thanks as well, looks good to me! As this is at +2, merging immediately.

jpelgrims-atmire and others added 21 commits July 31, 2019 11:57

Add leftWard/rightWard-Label to Relationship

172e5ec

Modified the Relationship so that it also has a leftWardLabel and a rightWardLabel for storing alternative labels (name variants). Also created an sql migration to add two columns for these labels to the relationship table.

Update PUT & POST endpoints for relationships to support name variants

bbb6dca

Add tests for relationship name variant

bb85c4f

Fix two small issues

c40dabf

Refactor PUT endpoint json parsing to make use of objectmapper

0377e0a

Moved RelationshipMetadata logic to a new service

1866ed2

intermediary commit

f17fcc2

Rename database column names for leftward/rightwardLabel

535847d

Merge branch 'feature-name-variants' of https://github.com/atmire/DSpace

db24de2

into feature-name-variants

Finished IT for name variants

d7a1015

leftwards label vs left item

8f7c7da

wording fixes

664dc00

checkstyle

afc9e5b

Merge branch 'feature-name-variants' of https://github.com/atmire/DSpace

c2cd25d

into feature-name-variants # Conflicts: # dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java # dspace-api/src/main/java/org/dspace/content/RelationshipMetadataServiceImpl.java

Fix issue causing tests to fail

6bf6336

Revert previous commit

e8fd1e0

Fixed tests

7eeb9f9

wording fixes

718340e

Rename leftwardLabel and rightwardLabel

ed17587

Renamed the leftwardLabel and rightwardlabel for Relationship to leftwardValue and rightwardValue. Renamed the same values for RelationshipType to leftwardType and rightwardType.

benbosman mentioned this pull request Aug 22, 2019

Rename relationship type properties DSpace/dspace-angular#461

Merged

Raf-atmire added 2 commits September 2, 2019 12:43

Changes type to typeName to avoid confusing, fixed checkstyle

ec3b07e

Merge remote-tracking branch 'dspace/master' into feature-name-variants

a0e18d5

Conflicts: dspace-api/src/main/java/org/dspace/content/ItemServiceImpl.java

tdonohue reviewed Sep 3, 2019

View reviewed changes

mwoodiupui reviewed Sep 4, 2019

View reviewed changes

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java Outdated Show resolved Hide resolved

mwoodiupui reviewed Sep 4, 2019

View reviewed changes

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java Outdated Show resolved Hide resolved

benbosman added 2 commits September 5, 2019 09:28

Change method names to make them easier to understand

0e45859

Add javadocs to complex private methods

Change method names to make them easier to understand

e8662d7

Add javadocs to complex private methods

tdonohue reviewed Sep 6, 2019

View reviewed changes

benbosman and others added 2 commits September 9, 2019 14:44

Change method names to make them easier to understand

46e8632

[Task 64911] added RelationshipMetadataService test

2de90e0

Raf-atmire force-pushed the feature-name-variants branch from 3bb715e to 2de90e0 Compare September 17, 2019 09:50

dimitrispie reviewed Sep 30, 2019

View reviewed changes

paulo-graca reviewed Sep 30, 2019

View reviewed changes

tdonohue reviewed Sep 30, 2019

View reviewed changes

Raf-atmire and others added 2 commits October 2, 2019 09:34

Applied feedback

85b88b0

comments

53ff1af

paulo-graca approved these changes Oct 2, 2019

View reviewed changes

tdonohue approved these changes Oct 2, 2019

View reviewed changes

tdonohue merged commit e638c47 into DSpace:master Oct 2, 2019

benbosman mentioned this pull request Oct 3, 2019

DS-4226 Improvements to Entity validation first pass #2522

Merged

benbosman deleted the feature-name-variants branch September 11, 2020 07:31

tdonohue added this to the 7.0beta1 milestone Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename properties and support for name variants #2488

Rename properties and support for name variants #2488

benbosman commented Aug 13, 2019 •

edited

tdonohue commented Aug 26, 2019

tdonohue left a comment

benbosman commented Sep 5, 2019

tdonohue left a comment

tdonohue Sep 6, 2019

benbosman Sep 9, 2019

benbosman Sep 17, 2019

dimitrispie left a comment

paulo-graca left a comment •

edited

tdonohue left a comment

paulo-graca left a comment

tdonohue left a comment

Rename properties and support for name variants #2488

Rename properties and support for name variants #2488

Conversation

benbosman commented Aug 13, 2019 • edited

tdonohue commented Aug 26, 2019

tdonohue left a comment

Choose a reason for hiding this comment

benbosman commented Sep 5, 2019

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue Sep 6, 2019

Choose a reason for hiding this comment

benbosman Sep 9, 2019

Choose a reason for hiding this comment

benbosman Sep 17, 2019

Choose a reason for hiding this comment

dimitrispie left a comment

Choose a reason for hiding this comment

paulo-graca left a comment • edited

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

paulo-graca left a comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

benbosman commented Aug 13, 2019 •

edited

paulo-graca left a comment •

edited