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

support section variable in manual url #8494

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

wangf1122
Copy link
Collaborator

The manual url can be more flexible. In some places, the url can be like https://something/subsection?lang=eng ect. So I am adding section variable as part of the url pattern and the front end Javascript will handle this pattern as well.

image

Checklist

  • I have read the contribution guidelines
  • Pull request provided for main branch, backports managed with label
  • Good housekeeping of code, cleaning up comments, tests, and documentation
  • Clean commit history broken into understandable chucks, avoiding big commits with hundreds of files, cautious of reformatting and whitespace changes
  • Clean commit messages, longer verbose messages are encouraged
  • API Changes are identified in commit messages
  • Testing provided for features or enhancements using automatic tests
  • User documentation provided for new features or enhancements in manual
  • Build documentation provided for development instructions in README.md files
  • Library management using pom.xml dependency management. Update build documentation with intended library use and library tutorials or documentation

@wangf1122 wangf1122 marked this pull request as ready for review November 8, 2024 14:39
@ianwallen ianwallen added this to the 4.4.7 milestone Nov 11, 2024
@ianwallen
Copy link
Contributor

I wonder if we want to make {{section}} optional and if it does not exists then append section at the end. (Which seem to be the current logic)

Of if we just want to treat is as required if want to use it.
i.e. should we support url like http://example.com?lang=en that would not include the section.
Otherwise, the current implementation will always append the section so http://example.com?lang=en would turn into http://example.com?lang=en/section which would cause issues. And to get around this we would need to add http://example.com?lang=en&dummy={{section}} to add a dummy section that is not used.

If we do want {{section}} to be mandatory to be used then I suspect that the section will need to be added to existing urls

UPDATE Settings SET value = CONCAT(value, '{{section}}') WHERE name = 'system/documentation/url';

Also you will need to update the following.

INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('system/documentation/url', 'https://docs.geonetwork-opensource.org/{{version}}/{{lang}}', 0, 570, 'n');
To

INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('system/documentation/url', 'https://docs.geonetwork-opensource.org/{{version}}/{{lang}}/{{section}}', 0, 570, 'n');

What do other think?

@ianwallen ianwallen requested a review from josegar74 November 11, 2024 13:49
@josegar74
Copy link
Member

@ianwallen I think it is fine if the placeholder is optional

Copy link
Member

@josegar74 josegar74 left a comment

Choose a reason for hiding this comment

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

The change works, but with the current code this kind of URL doesn't work:

https://something/subsection?lang=eng

The lang placeholder replacement checks for /{{lang}} instead of {{lang}}. This is not changed in the PR, but not sure why the original code has the /.


The official manual adds the language as part of the URL path, not as a parameter:

https://docs.geonetwork-opensource.org/4.4/fr/overview/

I assume you have some customised documentation that uses language as a parameter?

@wangf1122 wangf1122 force-pushed the main.section.variable.manual.url branch from 2e70774 to 0b000ce Compare November 13, 2024 20:12
@wangf1122 wangf1122 requested a review from josegar74 November 13, 2024 20:53
@wangf1122
Copy link
Collaborator Author

The change works, but with the current code this kind of URL doesn't work:

https://something/subsection?lang=eng

The lang placeholder replacement checks for /{{lang}} instead of {{lang}}. This is not changed in the PR, but not sure why the original code has the /.

The official manual adds the language as part of the URL path, not as a parameter:

https://docs.geonetwork-opensource.org/4.4/fr/overview/

I assume you have some customised documentation that uses language as a parameter?

@josegar74

The original has / is because it doesn't like to parse English into the default document.

For example:

https://docs.geonetwork-opensource.org/4.2/user-guide/describing-information/creating-metadata/

vs French version

https://docs.geonetwork-opensource.org/4.2/fr/user-guide/describing-information/creating-metadata/.

Now if we ignore that "/" logic, we will end up with default English url as:
https://docs.geonetwork-opensource.org/4.2//user-guide/describing-information/creating-metadata/ which seems to be fine. I could make such logic by ignoring such slash

if (baseUrl.includes("{{section}}")) {
helpPageUrl = baseUrl.replace("{{section}}", page);
} else {
helpPageUrl = baseUrl + "/" + page;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add the "/"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the default original code.

The default manual.json of each section contains was appended at the end of the url and they contain no such "/". So the Javascript has to add them

"creating-metadata": "user-guide/describing-information/creating-metadata/",

@@ -152,20 +152,30 @@
if (gnGlobalSettings.lang !== "en") {
baseUrl = scope.helpBaseUrl.replace("{{lang}}", gnGlobalSettings.lang);
} else {
baseUrl = scope.helpBaseUrl.replace("/{{lang}}", "");
baseUrl = scope.helpBaseUrl.replace("{{lang}}", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be "."

i.e. there are 2 urls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I think about it, it does not make sense to user "." either because someone could use ?lang={{lang}} and it would be odd to add "." and ?lang=. would not make sense!
So I guess "" is better in this case.
Maybe there could be another replace on the url after the replace to remove all the double "//"

As for why there is a odd case where lang = ""
What if it treated gnGlobalSettings.lang !== "en" as if gnGlobalSettings.lang = null?
Then {{lang?en}} could mean that if lang is null then default to en
So you could add another replacement for the following.
baseUrl = scope.helpBaseUrl.replace("{{lang?en}}", "");

So gn could use
https://docs.geonetwork-opensource.org/4.2/{{lang}}/api/search/
And other sites that always want to have a language value for language could use
https://example.com/{{lang?en}}/api/search/?lang={{lang?en}} and it would result to en value.
But this would still be odd logic.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@josegar74
Copy link
Member

I would keep the replacement to empty string and replace // in the URL path.

Please check this change to apply it in your code:

          /**
           * Processes an URL removing // characters in the URL path.
           *
           * @param url
           * @returns {string}
           */
          var processUrl = function(url) {
            var urlToProcess = new URL(url);
            urlToProcess.pathname = urlToProcess.pathname.replace(/\/\//g, "/");
            return urlToProcess.toString();
          }

          scope.showHelp = function () {
          ...
           var helpPageUrl;
            if (baseUrl.includes("{{section}}")) {
              helpPageUrl = baseUrl.replace("{{section}}", page);
            } else {
              helpPageUrl = baseUrl + "/" + page;
            }

            helpPageUrl = processUrl(helpPageUrl);

            testAndOpen(helpPageUrl).then(
              function () {},
              function () {
                var baseUrl = scope.helpBaseUrl
                  .replace("{{lang}}", "")
                  .replace("{{version}}", scope.applicationVersion);
                var helpPageUrl;
                if (baseUrl.includes("{{section}}")) {
                  helpPageUrl = baseUrl.replace("{{section}}", page);
                } else {
                  helpPageUrl = baseUrl + "/" + page;
                }

                helpPageUrl = processUrl(helpPageUrl);

                testAndOpen(helpPageUrl);
              }
              ...

@wangf1122
Copy link
Collaborator Author

@josegar74

This approach works fine. I did apply this into the code, please check

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.

4 participants