-
Notifications
You must be signed in to change notification settings - Fork 60
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
displayInfo and displayAttribute tags for datasets.xml #240
Conversation
TODO:
|
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.
This is a good start. I have a couple of comments overall:
It'd be good to add tests. In particular a test that the parameters are parsed correctly and a test that they are used correctly. TopLevelHandlerTests.java would likely be a good spot for the parsing test (do note the comment about making sure EDStatic is not modified for other tests, so for now that means setting these values back to default when the test is complete). As for testing the usage, most likely that will be best done in JettyTests.java. You'll probably need to modify EDDTestDataset.generateDatasetsXml to include the new top level parameters and then in a new test in the JettyTests.java file, use SSR.getUrlResponseStringUnchanged to load a page for a dataset (that has the proper attributes) and make sure the new attributes are properly rendered.
This is currently only for the SAX parser and currently requires setting useSaxParser to true as well. That's fine but that should definitely be documented somewhere along with how to use it. Would you be able to include a short markdown file explaining the feature and that it's SAX parser only?
A couple of comments in the code as well.
String attribute = EDStatic.displayAttributeAr[i]; | ||
|
||
// Skip "summary" and "license" as they are already handled | ||
if ("summary".equals(attribute) || "license".equals(attribute)) { |
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.
Summary and license should be included in this general version. That does mean removing their code above.
It'd be ideal to have their display attribute localized (using the EDStatic.EDDSummmaryAr[language] for summary and similar for license).
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 meant that summary and license should be included in this loop.
@@ -3820,6 +3821,39 @@ public void writeHtmlDatasetInfo( | |||
+ "</table>\n"); | |||
} | |||
|
|||
private String getDisplayInfo(int language, String loggedInAs) { | |||
if (EDStatic.displayAttributeAr.length != EDStatic.displayInfoAr.length) { |
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.
A good spot to do the validation would be in LoadDatasets.java just after the parsing is complete:
https://github.com/ERDDAP/erddap/blob/main/WEB-INF/classes/gov/noaa/pfel/erddap/LoadDatasets.java#L229
If the validation fails, it should log the issue and then set the EDStatic.displayAttributeAr and EDStatic.displayInfoAr to the defaults.
It's probably worth keeping a check here to make sure they match just before using them.
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.
There should be a log here as well.
I made some progress but for the jetty tests when I run the command |
The JettyTests are part of the integration tests, You can run them with mvn verify, though that will run all of the integration tests. You should be able to run:
or
to run just the JettyTests I've seen both in documentation, haven't verified which one works myself. |
|
@@ -229,6 +229,13 @@ public void run() { | |||
int nTry = nTryAndDatasets[0]; | |||
int nDatasets = nTryAndDatasets[1]; | |||
|
|||
// validation for displayInfo and displayAttribute Tags |
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 we put the validation in a function (validateDatasetsXmlResults or something). I know it's only one check right now, but making it a function would help make a place for future validation to be placed.
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 haven't run an individual integration test, so if those commands from the documentation are not working I'd need to look into it.
Adding to SimpleXmlParser logic would be good, though not needed, I'm happy with this being a feature to try to encourage people to update to the SAX parrser.
Yes, a file with.md syntax. I'm in the process of migrating all of the documentation to markdown format so that we can use Docusaurus for our documentation site.
Code code is on the right track, I've added some comments.
String attribute = EDStatic.displayAttributeAr[i]; | ||
|
||
// Skip "summary" and "license" as they are already handled | ||
if ("summary".equals(attribute) || "license".equals(attribute)) { |
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 meant that summary and license should be included in this loop.
I am not sure how I can add Was there some other way you were referring to implement this? |
Yes, that is what I meant. The reasoning is to simplify the code (summary and license behave almost identically to these other attributes), make the behavior more predictable (having all of the items that appear the same way in the UI behave the same way in configuration), and allow for more admin control (admins can choose to change the order - maybe they want it to be citation,license,summary - or skip having a summary if they would like to). |
|
|
The documentation is great, Thanks for writing that! I've sent a pull request that simplifies the code a little bit and fixes one of the JettyTests. Let me know if you have questions on those changes. |
SImplify code in getDisplayInfo and add the new attributes to the Jet…
Issue #134
<displayInfo>
and<displayAttribute>
are now available as global level tags indatasets.xml
,
displayAttribute
is the attribute name from the top leveladdAttributes
tag of a dataset whose value is displayed in the tooltipdisplayInfo
should contain what the word should be beside the tooltipdatasets.xml
the default values are "summary" and "license" which work the same