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

Update skin.xsl so that it works better in nojs mode. #7015

Merged
merged 8 commits into from
Jul 21, 2023

Conversation

ianwallen
Copy link
Contributor

Going to a link like the following

https://apps.titellus.net/geonetwork/srv/api/records/78f6187d-dac3-4721-8936-7682d934cd7b?language=fre

Can be used to link directly to a metadata records and it uses the skin.xsl however there are are couple problem.

Text and translation is missing.
Authentication link is always displayed.
It does not use node configurations
And there are issues with the links due to ../../ reference and in this sample link ../../ will go to generate /srv/srv/

So this PR does the following

  • Added missing translation
  • Check authentication before displaying login link
  • Use node configurations
  • Fix issue with base url for url like /geonetwork/srv/api/records/{uuid}?language=eng

- Added missing translation
- Check authentication before displaying login link
- Use node configurations
- Fix issue with base url for url like /geonetwork/srv/api/records/{uuid}?language=eng
@ianwallen
Copy link
Contributor Author

This is related to issue #6934

@josegar74 josegar74 added this to the 4.2.4 milestone Apr 21, 2023
@josegar74 josegar74 self-requested a review April 21, 2023 07:08
@fxprunayre fxprunayre modified the milestones: 4.2.4, 4.2.5 May 10, 2023
try {
translations.addContent(new Element(entry.getKey()).setText(entry.getValue()));
} catch (Exception e) {
System.out.println("Failed to add translation key for \"" + entry.getKey() + "\"=\"" + entry.getValue() + "\". " + e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be changed to log the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to log the exception

Element translations = new Element("translations");
Map<String, String> translationMap = new JSONLocCacheLoader(configurableApplicationContext, fparams.context.getLanguage()).call();
for (Map.Entry<String, String> entry : translationMap.entrySet()) {
// Skip keys that are not alpha numeric only including "." - otherwhise chars like : ? +... cause problem when creating element.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you indicate some translation including such values in the keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comments to show some sample keys that cause issues.

web/src/main/webapp/xslt/skin/default/skin.xsl Outdated Show resolved Hide resolved
@ianwallen ianwallen requested a review from josegar74 June 18, 2023 10:28
@ianwallen
Copy link
Contributor Author

I just corrected an issue that was happening when the ui configuration was not set.

In this case /root/request/ui was returning null and was not using the correct defaults.

Maybe there should be a future fix for /root/request/ui to always have a value. Maybe it should return the same default that is defined in CatController.js - defaultConfig?

@fxprunayre fxprunayre modified the milestones: 4.2.5, 4.2.6 Jul 7, 2023
title="{$t/reset}">
<i class="fa fa-times">&#160;</i>
</a>
<xsl:if test="/root/search/response">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this snippet is relevant anymore, the code looks specific to the 3.X versions, for example: https://github.com/geonetwork/core-geonetwork/pull/7015/files#diff-0288f54a93a5cbb90245269c2610c967bea1bfedad167b40f57270c08f9e0cc1R176

In any case it's not new in the PR.

@ianwallen ianwallen merged commit c1091bc into geonetwork:main Jul 21, 2023
ianwallen added a commit that referenced this pull request Jul 21, 2023
* Update skin so that it works better in nojs mode.
- Added missing translation
- Check authentication before displaying login link
- Use node configurations
- Fix issue with base url for url like /geonetwork/srv/api/records/{uuid}?language=eng

* Update web/src/main/webapp/xslt/skin/default/skin.xsl

Co-authored-by: Jose García <josegar74@gmail.com>

* Added logging and sample data that causes issues.

* If no configuration has been setup then /root/request/ui will be null. In this case use defaults identified from CatController.js - defaultConfig

* Update typo in comments

---------

Co-authored-by: Jose García <josegar74@gmail.com>
Co-authored-by: Ian <ian.allen@dfo.mpo.gc.ca>
ianwallen added a commit that referenced this pull request Jul 21, 2023
* Update skin so that it works better in nojs mode.
- Added missing translation
- Check authentication before displaying login link
- Use node configurations
- Fix issue with base url for url like /geonetwork/srv/api/records/{uuid}?language=eng

* Update web/src/main/webapp/xslt/skin/default/skin.xsl

Co-authored-by: Jose García <josegar74@gmail.com>

* Added logging and sample data that causes issues.

* If no configuration has been setup then /root/request/ui will be null. In this case use defaults identified from CatController.js - defaultConfig

* Update typo in comments

---------

Co-authored-by: Jose García <josegar74@gmail.com>
Co-authored-by: Ian <ian.allen@dfo.mpo.gc.ca>
@ianwallen ianwallen deleted the update_nojs_skin branch November 30, 2023 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants