4684 metadatablock internationalization#5111
4684 metadatablock internationalization#5111JayanthyChengan wants to merge 34 commits intoIQSS:developfrom
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.
When would this be null?
There was a problem hiding this comment.
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.
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.
In both MetadataBlock and Roles case, I haven't removed any columns in the db.
| public String getDisplayName() { | ||
| if (isHasParent() && !parentDatasetFieldType.getTitle().equals(title)) { | ||
| return parentDatasetFieldType.getTitle() + " " + title; | ||
| String s = ""; |
There was a problem hiding this comment.
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.
I will change the variable name.
There was a problem hiding this comment.
renamed and committed the code
| 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.
Can you explain the need for this?
There was a problem hiding this comment.
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.
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.
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 |
…Json_MetadataBlock()
This reverts commit cc69db0
…atablock-internationalization
This reverts commit 58fc744
There was a problem hiding this comment.
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 |
5 participants
Related Issues
Pull Request Checklist