[FIX JENKINS-35845] Internationalisation for Blue Ocean and JDL #2586

scherler · 2016-10-11T09:23:36Z

Allow plugins to export i18n resources.

Alternative to #2400 related to JENKINS-35270.

Created the PR because of [JENKINS-35845] Internationalisation for Blue Ocean and JDL

@jenkinsci/code-reviewers @reviewbybees

Signed-off-by: Thorsten Scherler <[email protected]>

Currently issues a 200 with an "error" response payload. This change still issues the error response payload, but also sets the HTTP response. Signed-off-by: Thorsten Scherler <[email protected]>

ghost · 2016-10-11T09:39:49Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

scherler · 2016-10-11T14:01:10Z

@tfennelly can you have a look on the above since you appear the original author.

tfennelly · 2016-10-12T13:26:28Z

core/src/main/java/jenkins/I18n.java

        } catch (Exception e) {
-            return HttpResponses.errorJSON(e.getMessage());
+            return HttpResponses.errorJSON(e.getMessage(), HttpServletResponse.SC_INTERNAL_SERVER_ERROR);


I see what you are doing here and it makes sense, but strictly speaking is not needed for fixing the issue wrt loading bundles from plugins, or am I missing something?

@tfennelly you are right strictly this is not needed.

Better to submit as a separate PR then

I will refactor to drop the part on SC_INTERNAL_SERVER_ERROR

tfennelly · 2016-10-12T13:27:28Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+                    }
+                }
+            }
+        }


I wonder should we be specifying the actual plugin name as a parameter on the rest endpoint?

I suppose this is ok ... keeps the code more simple and is only executed once (if the bundle is not in the cache).

tfennelly · 2016-10-12T13:29:43Z

test/src/test/java/jenkins/I18nTest.java

+            JSONObject response = jenkinsRule.getJSON("i18n/resourceBundle?baseName=com.acme.XyzWhatever").getJSONObject();
+        } catch (Exception e) {
+            Assert.assertNotNull(e);
+        }


Hmmm .... maybe check for something more specific?

I just looked and I think you should get a HtmlUnit exception (FailingHttpStatusCodeException I think) that you can check i.e. check for an expected exception Vs just any exception. If it is FailingHttpStatusCodeException, you should still be able to get the actual response and perform the same checks that were removed, I think.

I do not feel this test is enough for the change

oleg-nenashev

Several 🐜s in the implementation and definitely not enough tests. IIMHO 🐛

oleg-nenashev · 2016-10-12T15:47:13Z

core/src/main/java/hudson/util/HttpResponses.java

+     *
+     * @since TODO
+     */
+    public static HttpResponse errorJSON(@Nonnull String message, int errorCode) {


Maybe YAGNI, but OK

oleg-nenashev · 2016-10-12T15:48:02Z

core/src/main/java/hudson/util/HttpResponses.java

+         * @param statusCode The status code.
+         * @return {@link this} object.
+         */
+        public JSONObjectResponse setStatusCode(Integer statusCode) {


Maybe withStatusCode()?

oleg-nenashev · 2016-10-12T15:49:03Z

core/src/main/java/hudson/util/HttpResponses.java

@@ -109,6 +121,7 @@ public static HttpResponse errorJSON(@Nonnull String message) {
        private static final Charset UTF8 = Charset.forName("UTF-8");

        private final JSONObject jsonObject;
+        private Integer statusCode;


🐜 Add @CheckForNull.

oleg-nenashev · 2016-10-12T15:49:54Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+        if (bundle == null) {
+            // Not in Jenkins core. Check the plugins.
+            Jenkins jenkins = Jenkins.getInstance();
+            if (jenkins != null) {


This check is not required

Actually it is @oleg-nenashev the failure you have linked is provoked by that test not being in place anymore.

oleg-nenashev · 2016-10-12T15:51:45Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+            // Not in Jenkins core. Check the plugins.
+            Jenkins jenkins = Jenkins.getInstance();
+            if (jenkins != null) {
+                for (PluginWrapper plugin : jenkins.getPluginManager().getPlugins()) {


Ideally needs to be optimized. IIRC baseName usually contains /plugin/myPlugin, which may tweak the performance

not sure how to optimize though, some hints?

oleg-nenashev · 2016-10-12T15:54:46Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java


        bundleJSON = toJSONObject(bundle);
        bundles.put(bundleKey, bundleJSON);

        return bundleJSON;
    }

+    private static ResourceBundle getBundle(String baseName, Locale locale, ClassLoader classLoader) {


🐜 Add @ChewckForNull to return value and parameters if they are nullable

oleg-nenashev · 2016-10-12T15:55:11Z

test/src/test/java/jenkins/I18nTest.java

+            JSONObject response = jenkinsRule.getJSON("i18n/resourceBundle?baseName=com.acme.XyzWhatever").getJSONObject();
+        } catch (Exception e) {
+            Assert.assertNotNull(e);
+        }


I do not feel this test is enough for the change

… get the translations from the standard jenkins way, but needs changes in the core for now

… from oleg.

scherler

@oleg-nenashev I have created another test and fixed your comments.

scherler · 2016-10-13T10:36:20Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+            // Not in Jenkins core. Check the plugins.
+            Jenkins jenkins = Jenkins.getInstance();
+            if (jenkins != null) {
+                for (PluginWrapper plugin : jenkins.getPluginManager().getPlugins()) {


not sure how to optimize though, some hints?

oleg-nenashev · 2016-10-13T22:50:55Z

core/src/main/java/hudson/util/HttpResponses.java

@@ -23,28 +23,30 @@
 */
 package hudson.util;

+import java.io.File;


Please never use IDE's autoformat :(

oleg-nenashev · 2016-10-13T22:51:53Z

core/src/main/java/hudson/util/HttpResponses.java

+         * @param statusCode The status code.
+         * @return {@link this} object.
+         */
+        public JSONObjectResponse withStatusCode(Integer statusCode) {


Also @CheckForNull for parameter? NIT

oleg-nenashev · 2016-10-13T22:53:08Z

core/src/main/java/jenkins/I18n.java

        } catch (Exception e) {
-            return HttpResponses.errorJSON(e.getMessage());
+            return HttpResponses.errorJSON(e.getMessage(), HttpServletResponse.SC_INTERNAL_SERVER_ERROR);


oleg-nenashev · 2016-10-13T22:54:37Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+     * @return The bundle JSON.
+     * @throws MissingResourceException Missing resource bundle.
+     */
+    private static @Nullable


Strange formatting

Should be CheckForNull since it may happen on a common path

oleg-nenashev · 2016-10-13T22:55:09Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+        try {
+            return ResourceBundle.getBundle(baseName, locale, classLoader);
+        } catch (MissingResourceException e) {
+            // fall through and return null.


🐜 Must go to system log

oleg-nenashev · 2016-10-13T22:55:53Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+     * @param locale The Locale.
+     * @param classLoader The classLoader
+     * @return The bundle JSON.
+     * @throws MissingResourceException Missing resource bundle.


🐛 It's not being thrown from the method

scherler · 2016-10-14T20:06:52Z

@oleg-nenashev fixed your feedback

oleg-nenashev · 2016-10-15T15:38:09Z

Still 🐜 for unrelated formatting changes, but the changes itself LGTM. 🐝

scherler · 2016-10-17T10:12:26Z

@oleg-nenashev fixed all not releated formating changes

oleg-nenashev · 2016-10-18T15:10:13Z

Test failure seems to be related. The logic causes NPE: https://ci.jenkins.io/job/Core/job/jenkins/job/PR-2586/9/testReport/jenkins.util/ResourceBundleUtilTest/test_unknown_bundle/

oleg-nenashev · 2016-10-18T15:12:36Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+     * @param classLoader The classLoader
+     * @return The bundle JSON.
+     */
+    private static @CheckForNull ResourceBundle getBundle(@Nonnull String baseName, @Nonnull Locale locale, @Nonnull ClassLoader classLoader) {


Well, @jglick-style violates the recommended code style

@oleg-nenashev not sure what you mean

Well...

Style defined in https://wiki.jenkins-ci.org/display/JENKINS/Beginners+Guide+to+Contributing#BeginnersGuidetoContributing-CodeStyle

@CheckForNull private static ResourceBundle getBundle(@Nonnull String baseName, @Nonnull Locale locale, @Nonnull ClassLoader classLoader)

jglick-style:

private static @CheckForNull ResourceBundle getBundle(@Nonnull String baseName, @Nonnull Locale locale, @Nonnull ClassLoader classLoader)

@oleg-nenashev ok got ya, so should I fix the other occurrence of that in the file?

Yep, the file contains only one annotation style.

Then I'd just keep it as is and then perform a bulk refactoring when Jesse goes to a vacation without internet access :)

oleg-nenashev · 2016-10-18T15:13:46Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+            return ResourceBundle.getBundle(baseName, locale, classLoader);
+        } catch (MissingResourceException e) {
+            // fall through and return null.
+            logger.info(e.getMessage());


Add error to the logged message. Likely INFO is a bad logging level since it's an expected behavior sometimes. Will pollute logs

…le test by prevent NPE to happen

scherler · 2016-10-18T16:28:16Z

@oleg-nenashev your feedback is in this PR now.

scherler · 2016-10-20T14:54:02Z

@oleg-nenashev ping

oleg-nenashev · 2016-10-20T14:58:21Z

test/src/test/java/jenkins/I18nTest.java

+    }
+
+    @Test
+    public void test_baseName_plugin() throws IOException, SAXException {


Nice2have: Add @Issue() reference

oleg-nenashev

Looks good to me 🐝

oleg-nenashev · 2016-10-20T17:09:57Z

re-:bee:

aheritier · 2016-10-20T17:22:28Z

🐝

oleg-nenashev · 2016-10-20T17:27:04Z

@reviewbybees done
@jenkinsci/code-reviewers your turn

oleg-nenashev · 2016-10-22T10:21:51Z

Merging in order to get the fix in the next weekly.
Works fine according to the manual testing

olivergondza · 2016-11-08T14:29:46Z

core/src/main/java/jenkins/util/ResourceBundleUtil.java

+            return ResourceBundle.getBundle(baseName, locale, classLoader);
+        } catch (MissingResourceException e) {
+            // fall through and return null.
+            logger.warning(e.getMessage());


Does this really deserves warning? IIUC it is ok if particular plugin just does not provide it.

In case no plugin provide it we will have one exception thrown and N warning records logged. If it is provided in the last plugin, there will be N-1 warnings for no reason.

I think we just need to reduce the message severity to the FINE or FINEST level

@oleg-nenashev is this the only update that is blocking this from backporting?

"Blocking" - maybe. But we still need to agree regarding the feasibility of the backporting

Wait, will this print an average of dozens of messages every time a non-core resource is searched? It iterates over all plugins… If so, this should be changed in master as well.

Yes, it should. Creating the issue

https://issues.jenkins-ci.org/browse/JENKINS-39604

olivergondza · 2016-11-09T06:44:51Z

@i386, this is the only thing that made me hesitate with backporting this. It will go in with oleg's tuning.

* Load i18n resource bundles from plugins if not found in jenkins core Signed-off-by: Thorsten Scherler <[email protected]> * Issue 404 response for missing i18n resource bundles Currently issues a 200 with an "error" response payload. This change still issues the error response payload, but also sets the HTTP response. Signed-off-by: Thorsten Scherler <[email protected]> * [JENKINS-35845] Fix test since we return now a 404 * [JENKINS-35845] add test for getting locale from plugin. fix comments from oleg. * [JENKINS-35845] Fix description * [JENKINS-35845] Update PR with comments from Oleg * [JENKINS-35845] Add feedback from tom * eslint - formating changes and fix offences * eslint - formating changes and fix offences * [JENKINS-35845] remove code concerning 404 response. Fix resourceBundle test by prevent NPE to happen * [JENKINS-35845] Link to issue on which we introduced the test (cherry picked from commit ab16e52)

@Dario

* [JENKINS-35845] WIP first steps with i18n * eslint - formating changes and fix offences * [JENKINS-35845] first basic working version with 2 languages * [JENKINS-35845] WIP adding patched backend to investigate * [JENKINS-35845] Assumes jenkinsci/jenkins#2586 to be applied. We know get the translations from the standard jenkins way, but needs changes in the core for now * [JENKINS-35845] remove testing class * [JENKINS-35845] WIP fixing integration of jenkins i18n keys with dot in them * [JENKINS-35845] move i18n to core-js and using it from within web * [JENKINS-35845] follow jenkins convention/pattern for storing locale * [JENKINS-35845] WIP implement translation in core-js runbutton and toastUtil. Prefix translations with bo.web. remove sample code * [JENKINS-35845] WIP starting to translate dashboard * [JENKINS-35845] WIP added german translation and finished dashboard. Currently working on making moment respect the locale * [JENKINS-35845] Use latest jdl * [JENKINS-35845] fix locale retrieval * [JENKINS-35845] update stories to use the new i18n functions. Create story to test readableDate and timeDuration. In node this works fine now traking down wht not in dashboard * [JENKINS-35845] Add spanish translations * [JENKINS-35845] better translation for spanish (thank you @Dario) as well updated some german translations * [JENKINS-35845] Fix german translations with feedback from @daniel * [JENKINS-35845] Fix test view and finish translation * [JENKINS-35845] Fix german translation. fix result views. fix some lint issues. * [JENKINS-35845] create a compose function to wire different functions together * [JENKINS-35845] better documentation and remove debug string * [JENKINS-35845] add documentation about i18n and linking contributing and i18n docu in principal readme. * [JENKINS-35845] updte docu * [JENKINS-35845] Pass locale and translation function down the component tree. Fix links to not drop query parameters. * [JENKINS-35845] Fix links to not drop query * [JENKINS-35845] use the url config util to get correct path to jenkins * [JENKINS-35845] WIP security commit - refactor i18n class to support listener and subscribe to i18nChanges. Will allow to drop react specific integration via react-i18next. * [JENKINS-35845] WIP security commit * [JENKINS-35845] remove debug statement * [JENKINS-35845] fix import * Task/jenkins 35845 i18n key rename (#579) * [JENKINS-35845] rework i18n keys for home page * [JENKINS-35845] l10n for activity tab * [JENKINS-35845] l10n for branches tab * [JENKINS-35845] l10n for pull requests tab * [JENKINS-35845] l10n for run details -> pipeline * [JENKINS-35845] l10n for run details -> changes, tests, artifacts * [JENKINS-35845] l10n for run details header changes * [JENKINS-35845] pagination; fix typo causing error * [JENKINS-35845] order keys * [JENKINS-35845] WIP security commit to be able to merge master * eslint - formating changes and fix offences * [JENKINS-35845] Remove dep to snapshot-jenkins and implement fallback to default values. Fix tests of all related projects. * [JENKINS-35845] remove duplicate variable * [JENKINS-35845] fix test by adding polyfy again * eslint - formating changes and fix offences * [JENKINS-35845] Fix last test * [JENKINS-35845] fix tests for dashboard * [JENKINS-35845] sync version numbers

scherler added 2 commits October 11, 2016 11:12

Load i18n resource bundles from plugins if not found in jenkins core

2766eee

Signed-off-by: Thorsten Scherler <[email protected]>

Issue 404 response for missing i18n resource bundles

5641b66

Currently issues a 200 with an "error" response payload. This change still issues the error response payload, but also sets the HTTP response. Signed-off-by: Thorsten Scherler <[email protected]>

[JENKINS-35845] Fix test since we return now a 404

3ee57ae

kzantow changed the title ~~Jenkins 35845 Internationalisation for Blue Ocean and JDL~~ [FIX JENKINS-35845] Internationalisation for Blue Ocean and JDL Oct 11, 2016

tfennelly reviewed Oct 12, 2016

View reviewed changes

oleg-nenashev requested changes Oct 12, 2016

View reviewed changes

scherler added a commit to jenkinsci/blueocean-plugin that referenced this pull request Oct 13, 2016

[JENKINS-35845] Assumes jenkinsci/jenkins#2586 to be applied. We know…

92a421e

… get the translations from the standard jenkins way, but needs changes in the core for now

[JENKINS-35845] add test for getting locale from plugin. fix comments…

cf7d71b

… from oleg.

scherler commented Oct 13, 2016

View reviewed changes

[JENKINS-35845] Fix description

c05ac32

oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Oct 13, 2016

oleg-nenashev reviewed Oct 13, 2016

View reviewed changes

scherler added 2 commits October 14, 2016 21:15

[JENKINS-35845] Update PR with comments from Oleg

1fe461c

[JENKINS-35845] Add feedback from tom

40f1885

oleg-nenashev mentioned this pull request Oct 15, 2016

[FIXED JENKINS-35270] Support loading js localization bundles from plugins #2400

Closed

scherler added 2 commits October 17, 2016 12:08

eslint - formating changes and fix offences

72cbdbb

eslint - formating changes and fix offences

0b8c8cd

Merge remote-tracking branch 'origin/master' into JENKINS-35845

86f4c46

oleg-nenashev reviewed Oct 18, 2016

View reviewed changes

[JENKINS-35845] remove code concerning 404 response. Fix resourceBund…

0aab660

…le test by prevent NPE to happen

oleg-nenashev reviewed Oct 20, 2016

View reviewed changes

oleg-nenashev approved these changes Oct 20, 2016

View reviewed changes

[JENKINS-35845] Link to issue on which we introduced the test

46b483e

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Oct 21, 2016

oleg-nenashev merged commit ab16e52 into jenkinsci:master Oct 22, 2016

oleg-nenashev added a commit that referenced this pull request Oct 23, 2016

Changelog: Noting#2595, #2492, #2590 and #2586

12236a8

oleg-nenashev added a commit that referenced this pull request Oct 23, 2016

Changelog: Wrong link in #2586

bfa9cc3

olivergondza reviewed Nov 8, 2016

View reviewed changes

oleg-nenashev mentioned this pull request Nov 8, 2016

[FIXED JENKINS-39604] - ResourceBundleUtil#getBundle() should report misses on the low level #2627

Merged

[FIX JENKINS-35845] Internationalisation for Blue Ocean and JDL #2586

[FIX JENKINS-35845] Internationalisation for Blue Ocean and JDL #2586

Conversation

scherler commented Oct 11, 2016 • edited Loading

ghost commented Oct 11, 2016

scherler commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scherler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scherler commented Oct 14, 2016

oleg-nenashev commented Oct 15, 2016

scherler commented Oct 17, 2016

oleg-nenashev commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scherler commented Oct 18, 2016

scherler commented Oct 20, 2016

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Oct 20, 2016

aheritier commented Oct 20, 2016

oleg-nenashev commented Oct 20, 2016

oleg-nenashev commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i386 Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivergondza commented Nov 9, 2016

scherler commented Oct 11, 2016 •

edited

Loading

oleg-nenashev Oct 12, 2016 •

edited

Loading

i386 Nov 8, 2016 •

edited

Loading

oleg-nenashev Nov 8, 2016 •

edited

Loading