[VUFIND-1586] Clean up language keys for file import on lokalise.com

[lahmann]

Importing language files on lokalise.com results in errors aborting import due to some characters in the language keys. So far the following characters where identified: ()!?

Found language keys:

• Form Submitted!
• Need Help?
• Send us your feedback!
• Subject(s)

TODO:

• Merge Remove unused language string. #2599
• Figure out how to address Subject(s) -- separate PR to refactor EDS label translation to its own text domain?
• Resolve VUFIND-1586 when merging

[lahmann]

I fixed: Need Help? and Send us your feedback!

Form Submitted! and Subject(s) are not found nor in templates neither in other code - delete those keys?

[demiankatz]

Thanks for getting this started, @lahmann. A few things:

1.) I wonder if we should take this opportunity to use token-style strings instead of simply removing punctuation from non-token-style strings. Since this largely impacts form configuration, I'm interested in @EreMaijala's input. It's possible that this may also be an opportunity to differentiate use of the "Need Help?" string based on context (e.g. "help_header" vs. "need_help").

2.) The "Form Submitted!" string is indeed obsolete. I have opened #2599 to clean that up.

3.) The "Subject(s)" string is a tricky one; this was introduced as part of the EDS module. Since EDS does not support native i18n, we have to translate arbitrary strings coming back from the EDS API, and this is one of them. I think it would be helpful to handle these things in their own namespace (this functionality was implemented before we had namespaces), but refactoring everything appropriately may be challenging. Maybe we can take a first stab at that in a separate PR when time permits -- e.g. normalize EDS labels to remove punctuation and add a namespace prefix, perhaps with a fallback to the default namespace in case we miss something. I'll add a TODO checkbox about the issue.

[EreMaijala]

@demiankatz @lahmann
1.) Yes please. Token style is my preference whenever the English translation doesn't match the key.

[EreMaijala]

Let's use token style when the English translation doesn't match the translation key.

[demiankatz]

Sounds good. @lahmann, let me know if you plan to work on this or if you'd like some help. Also note that most of these changes can be made via VuFind's command-line language management tools, so if you're not familiar with those already, they'll likely save some time. Let me know if you need more details!

[lahmann]

I finally found some time to push things forward. The last commit is a draft of introducing translation tokens for the occurrences of "Need help" and - to introduce translation tokens for similar elements in the template - for all the headers in the footer columns. Let me know what you think and if I should continue with this approach.
In any case I would limit the scope of this pull request to the translation strings mentioned in the first comment. If the tokenization is agreed upon as good practise we could refactor the remaining translation strings to tokens in another pull request (but that would be a seriously huge task to accomplish including removing unneeded strings introduction of new domains etc.).

[lahmann]

Regarding the EDS strings: I'm not sure if we already have an agreed upon plan how to proceed. Moving EDS related strings into a domain could be done, but apart from c88529d which introduced several (generic) translation strings I have no clue which strings are EDS specific (apart from the tokens eds_*). Is there any more efficient way to filter EDS specific strings and sort those into a domain than scanning all commits for EDS related contributions?

[demiankatz]

Thanks, @lahmann -- looks like there are just a couple more details to address.

Regarding EDS, it's really tricky, because the API can send us entirely arbitrary label text. We have no comprehensive list, and the strings cannot be found in the code. It would be much better of the API itself were internationalized, but as far as I know, EBSCO has not made plans to do that, so we're left doing the best we can.

I think this is the key place where Subject(s) comes into play (though it may not be the only one -- I would have to do a more careful review of EDS templates to be sure): https://github.com/vufind-org/vufind/blob/dev/themes/bootstrap3/templates/RecordDriver/EDS/core.phtml#L67

One solution might be to create a translateEDSLabel view helper which normalizes the strings, searches for translations in first an EDS namespace and then the default namespace, and returns the raw string if no translation is found anywhere. It's not elegant, but I don't think we're going to find a totally elegant solution to this problem. As noted elsewhere, I'm willing to make an attempt at this in a separate PR if you like, though I'm not sure when I'll have time to do that -- December is going to be a very busy month for me due to local system migrations, so I'm not expecting to have a lot of time for VuFind development.

[lahmann]

Thanks for taking a look at my comment. So, the naming scheme for the tokens will do and I should go on - at least for the translation strings in question of this pull request?

Regarding EDS the suggested solution via an extra view helper sounds reasonable but without having access to an EDS system implementation would be quite difficult on our side. Should we introduce an EDS domain in this ticket as a workaround for the Subject(s) string or do think we should pause this pull request until the EDS issue is solved?

[demiankatz]

@lahmann, I found a few minutes to put together a proof of concept of an EDS view helper (see #2642) and would be interested in continuing to discuss EDS issues on that PR. However, in the meantime, I believe we can simply delete the Subject(s) string entirely. In my testing of EDS today, I could not find that string at all, and I strongly suspect that it is no longer used by the API. If I'm mistaken, it can always be reintroduced in #2642 in the future as part of the EDS namespace, but I don't think it will cause significant harm if it is missing in the meantime.

[lahmann]

Great, thanks for going ahead with the EDS view helper. I introduced the remaining translation tokens and removed Subject(s). If I'm not mistaken that should do for cleaning up and moving forward with lokalise.com import?

Afterthought: for more consistency I also introduced a token for the feedback form response (Thank you for your feedback.).

[demiankatz]

Thanks, @lahmann, I agree that we're nearly done. However, I did a quick search for all the language strings that have been removed and found one remaining reference to one of them (see below).

I'm also going to run the full test suite, just to be sure nothing has broken. I don't expect any problems, but if something fails, I'll post a follow-up comment.

I also see that @EreMaijala has an outstanding request for changes, so it would be good to get an approval from him as well once the final correction is made.

[demiankatz]

@lahmann, actually, there were some test failures -- looks like you may have gotten carried away with some of the string replacement in the integration tests, since they will return translated results rather than keys:

There were 3 failures:

1) VuFindTest\Mink\FeedbackTest::testFeedbackForm
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'feedback_response'
+'Thank you for your feedback.'

/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FeedbackTest.php:113
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:99
phpvfscomposer:///usr/local/vufind/vendor/phpunit/phpunit/phpunit:97

2) VuFindTest\Mink\FeedbackTest::testFeedbackFormWithCaptcha
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'feedback_response'
+'Thank you for your feedback.'

/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FeedbackTest.php:142
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:99
phpvfscomposer:///usr/local/vufind/vendor/phpunit/phpunit/phpunit:97

3) VuFindTest\Mink\FeedbackTest::testIntervalCaptcha
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'feedback_response'
+'Thank you for your feedback.'

/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FeedbackTest.php:182
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:99
phpvfscomposer:///usr/local/vufind/vendor/phpunit/phpunit/phpunit:97

FAILURES!
Tests: 2192, Assertions: 11130, Failures: 3.

[lahmann]

Time flies... Thanks for the review, I fixed the tests and added the missing "Find more" translation string for the collection template. I named it collection_next_page_label as there where at least some collection_* tokens present.

[demiankatz]

@lahmann, I took a closer look at the way the collection_next_page_label was being used, and it seemed to me entirely unhelpful/unnecessary -- it provided somewhat out-of-context mouseover text on the icon inside the buttons that link to collection contents. See this screen shot:

I just went ahead and deleted it entirely (see 5addc62), as I don't think it was really helping anyone.

Happy to put something else back in there if @crhallberg or others think it would be important/helpful.

I've also updated the changelog with a summary of what we've done here, so everything is documented once we decide to go ahead and merge this. I'll leave it open a little longer for discussion first, though.

[demiankatz]

As long as nobody objects to my collections changes above, I believe this is ready to merge. I've tested all the impacted configurations and templates and everything displays correctly. I'll just await feedback for a few days before hitting the merge button.

[lahmann]

I have no objections either.