-
Notifications
You must be signed in to change notification settings - Fork 500
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
4684 metadatablock internationalization #5111
4684 metadatablock internationalization #5111
Conversation
…orresponding metadata block property files
…atablock-internationalization
…rom mvn test cases
…rom mvn test cases
…atablock-internationalization # Conflicts: # src/main/java/edu/harvard/iq/dataverse/util/BundleUtil.java
…atablock-internationalization
public String getStrValue() | ||
{ | ||
String key = strValue.toLowerCase().replace(" " , "_"); | ||
if(getDatasetFieldType().getMetadataBlock() == null){ |
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.
When would this be null?
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.
During mvn built, all the below test cases were failing due to getMetadataBlock is returning null. So, I handled it with if/else to resolve it .
DatasetFieldValueValidatorTest.testIsValid:110 » NullPointer
DDIExporterTest.testCitation:149 » NullPointer
SchemaDotOrgExporterTest.testExportDataset:155 » NullPointer
FeedbackUtilTest.testGatherFeedbackOnDataset:232 » NullPointer
FeedbackUtilTest.testGatherFeedbackOnDatasetNoContacts:280 » NullPointer
FeedbackUtilTest.testGatherFeedbackOnFile:353 » NullPointer
FeedbackUtilTest.testGatherFeedbackOnFileNoContacts:421 » NullPointer
BriefJsonPrinterTest.testJson_DatasetVersion:44 » NullPointer
JsonParserTest.testCompoundRepeatsRoundtrip:135 » NullPointer
JsonParserTest.testControlledVocalNoRepeatsRoundTrip:156 » NullPointer
JsonParserTest.testControlledVocalRepeatsRoundTrip:169 » NullPointer
JsonParserTest.testPrimitiveNoRepeatesFieldRoundTrip:215 » NullPointer
JsonParserTest.testPrimitiveRepeatesFieldRoundTrip:229 » NullPointer
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.
Similar to the Missing resource, it'd be ideal if we could understand what is happening here. One concern is that we'll now have the text in multiple places and one question I was going to ask is whether you removed the text and columns from the db (just leaving the bundle key - this is true for this and for roles)
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.
In both MetadataBlock and Roles case, I haven't removed any columns in the db.
@@ -492,10 +510,17 @@ public int compareTo(DatasetFieldType o) { | |||
} | |||
|
|||
public String getDisplayName() { | |||
if (isHasParent() && !parentDatasetFieldType.getTitle().equals(title)) { | |||
return parentDatasetFieldType.getTitle() + " " + title; | |||
String 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.
let's use a better variable name than "s" (also and there are several such places, when would metadatablock be null? if nowhere, I think we can simplify the logic here)
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.
I will change the variable name.
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.
renamed and committed the code
@@ -65,6 +69,9 @@ public void testJson_MetadataBlock() { | |||
assertEquals("metadata_block_name", res.getString("name")); | |||
assertEquals(1, res.getInt("id")); | |||
assertEquals(3, res.keySet().size()); | |||
} catch (MissingResourceException e) { |
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.
Can you explain the need for this?
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.
BriefJsonPrinterTest.testJson_MetadataBlock:62 » MissingResource Can't find bu...
The above testcase throw MissingResource exception, since BundleUtil cannot find the property file with metadata block name
mtb.setName("metadata_block_name");
so, I just handled it with try/catch
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.
Does this basically render the test as not being run? I'd rather not "hide" the test; either we figure out how to make it work or we may want to consider removing it (of course that is not ideal)? Did you do any research to why this error was happening (I know you sent me an e-mail asking, but I wasn't sure either)
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.
The reason for the error is due to the missing file "metadata_block_name.properties" , so added it and also it cleared the exception. Please check the latest commit.
…atablock-internationalization
…atablock-internationalization
controlledvocabulary.contributorType.data_curator=Data Curator | ||
controlledvocabulary.contributorType.data_manager=Data Manager | ||
controlledvocabulary.contributorType.editor=Editor | ||
controlledvocabulary.contributorType.funder=Funder |
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.
"Funder" would be handy over at 043080d#r30869150
…Json_MetadataBlock()
This reverts commit cc69db0
…atablock-internationalization
This reverts commit 58fc744
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.
The main thing I see needing work is how we have put the internationalization into our Entity getters. Basic getter/setters shouldn't be overloaded in this way, if we need to show different language values in the UI there should be special methods to show that.
I think this would help with some of the checks that seem to be in place to make the tests pass, if we stop overloading those getters with logic they will get and set the same values. The downside is that the UI logic moves outside the tests, but it makes more sense to not test these at all than to test them in a way that seems flawed.
@matthew-a-dunlap: Thanks for your review. |
@JayanthyChengan Thanks for checking in! That looks like the right structure to me, I think its good to continue on and do the same for the other fields. Thanks again |
Related Issues
Pull Request Checklist