Skip to content
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

Conversation

JayanthyChengan
Copy link
Contributor

Related Issues

Pull Request Checklist

  • Merged latest from "develop" and resolved conflicts

@coveralls
Copy link

coveralls commented Sep 27, 2018

Coverage Status

Coverage decreased (-0.002%) to 15.628% when pulling 4eed58f on scholarsportal:4684-metadatablock-internationalization into 2bac293 on IQSS:develop.

public String getStrValue()
{
String key = strValue.toLowerCase().replace(" " , "_");
if(getDatasetFieldType().getMetadataBlock() == null){
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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)

Copy link
Contributor Author

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 = "";
Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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)

Copy link
Contributor Author

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.

pdurbin referenced this pull request Oct 11, 2018
controlledvocabulary.contributorType.data_curator=Data Curator
controlledvocabulary.contributorType.data_manager=Data Manager
controlledvocabulary.contributorType.editor=Editor
controlledvocabulary.contributorType.funder=Funder
Copy link
Member

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

Copy link
Contributor

@matthew-a-dunlap matthew-a-dunlap left a 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.

@JayanthyChengan
Copy link
Contributor Author

@matthew-a-dunlap: Thanks for your review.
Submitted a new PR #5290 , and I added a new method for the displayname field in MetadataBlock.java, Please review the changes and if it's okay, I will work on title, description,watermark fields in DatasetFieldType and so on. Thanks

@matthew-a-dunlap
Copy link
Contributor

@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

@JayanthyChengan
Copy link
Contributor Author

Latest PR is #5290 , closing #5111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants