1279195 - Update error docs URL to be language-agnostic and add GA params

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Julien G]

When an error occurs in the console, the link targets the English doc page (e.g. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/JSON_bad_parse).

Ideally, this would change according to the locale of Firefox: if the user is on a French version, the links should point to https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Errors/JSON_bad_parse.

In other words, the "baseURL" in https://hg.mozilla.org/mozilla-central/file/b9e7c2e445f4/devtools/server/actors/errordocs.js should change according to the locale.

I'll try to find the way to do so in the next days but don't hesitate to take on this issue :)

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

You could change the url to https://developer.mozilla.org/docs/Web/JavaScript/Reference/Errors/JSON_bad_parse and have MDN auto-detect the locale based on browser language.

Comment 2

[Julien G]

Indeed :), I'll setup a build and fiddle with the dev tools contribution docs to make this my first patch :)

Comment 3

[Florian Scholz (Open Web Docs)]

There is also a demand to get Google Analytics data here:

So, something like: 
const baseURL = "https://developer.mozilla.org/docs/Web/JavaScript/Reference/Errors/";
const params = "?utm_source=mozilla&utm_medium=firefox-console-errors&utm_campaign=default";

and then

return baseURL + doc + params;

Comment 4

[Florian Scholz (Open Web Docs)]

Attachment: Bug 1279195 - Update error doc URL and add GA parameters

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8762853 [details] [diff] [review]
Bug 1279195 - Update error doc URL and add GA parameters

Review of attachment 8762853 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/webconsole/webconsole.js
@@ +1604,5 @@
>  
>      let urlNode = this.document.createElementNS(XHTML_NS, "a");
>      urlNode.className = "url";
>      urlNode.setAttribute("title", request.url);
> +    urlNode.href = request.url + ErrorDocs.GA_PARAMS;

This is the URL that shows the request path and when clicking opens the network panel.  GA_PARAMS shouldn't be added here

@@ +1699,5 @@
>          url = TRACKING_PROTECTION_LEARN_MORE;
>          break;
>        default:
>          // If all else fails check for an error doc URL.
> +        url = ErrorDocs.GetURL(scriptError.errorMessageName).split("?")[0];

This will throw if GetURL returns undefined.  I think the call to GetURL should not automatically append the GA_PARAMS.  Because right now we are appending the params, then stripping them off, then reappending them.

Might need to make minor updates to the test for that to change, but it'll be easier to follow.

Comment 6

[Florian Scholz (Open Web Docs)]

Attachment: Bug 1279195 - Update error doc URL and add GA parameters

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8764044 [details] [diff] [review]
Bug 1279195 - Update error doc URL and add GA parameters

Review of attachment 8764044 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/webconsole/jsterm.js
@@ +10,5 @@
>    require("devtools/shared/webconsole/utils");
>  const promise = require("promise");
>  const Debugger = require("Debugger");
>  const Services = require("Services");
> +const ErrorDocs = require("devtools/server/actors/errordocs");

The client shouldn't be taking on a server dependency.  This all points to a problem that we are sometimes getting the URLs from the client and sometimes from the server.  Sorry about that, it's not really your fault but I should fix this now before we go any further down this path since I think it's leading us to a non-optimal solution here.

I will fix that problem and send over a patch that you can build on top of, appending the GA_PARAMS onto the URL in the server as you  originally planned.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Bug 1279195 - Move error message URL onto server;

Review commit: https://reviewboard.mozilla.org/r/60104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60104/

Comment 9

[Brian Grinstead [:bgrins]]

Florian, your changes can be applied on top of the patch I've attached, where the GA_PARAMS is appended on the server and I think the only frontend changes that would be needed then are:

a) fixing tests that are looking for a URL without the params
b) stripping off any query string params if you don't want them to show up in the title

Comment 10

[Florian Scholz (Open Web Docs)]

Attachment: bug1279195.patch

Comment 11

[Florian Scholz (Open Web Docs)]

Thanks so much, Brian! I posted a new patch.

There is still one more link (MIXED_CONTENT_LEARN_MORE) on the client side, but maybe we can address that one as a follow-up.

I've also updated some MDN URLs as they have been moved and point to redirects.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Florian Scholz [:fscholz] (MDN) from comment #11)
> Thanks so much, Brian! I posted a new patch.
> 
> There is still one more link (MIXED_CONTENT_LEARN_MORE) on the client side,
> but maybe we can address that one as a follow-up.

Yes, that's what I was thinking.  I believe that's only for net request messages (which are off by default) and you'll still get the params if it's a mixed content warning coming directly from platform.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8764233 [details] [diff] [review]
bug1279195.patch

Review of attachment 8764233 [details] [diff] [review]:
-----------------------------------------------------------------

So the changes here look good to me in general.  Going to wait to make sure my first patch is good to go before saying r+ though.  Also, please have a look at this search: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Fwebconsole%2Ftest+https%3A%2F%2Fdeveloper.mozilla.org&redirect=true.  I expect all of these expected URLs will need to be updated with the ga params.

Comment 14

[Lin Clark [:linclark]]

Comment on attachment 8764058 [details]
Bug 1279195 - Move error message URL onto server;

https://reviewboard.mozilla.org/r/60104/#review57080

Tested using this test page: https://www.bennish.net/mixed-content.html. Worked as expected. It looks like there are some ESLint issues, so just make sure it's green on try before committing.

Just one minor note below, feel free to fix or not fix as you choose.

::: devtools/server/actors/errordocs.js:52
(Diff revision 1)
>    JSMSG_UNTERMINATED_STRING: "Unterminated_string_literal",
>    JSMSG_NOT_CONSTRUCTOR: "Not_a_constructor",
>    JSMSG_CURLY_AFTER_LIST: "Missing_curly_after_property_list",
>  };
>  
> -exports.GetURL = (errorName) => {
> +const MIXED_CONTENT_LEARN_MORE = "https://developer.mozilla.org/docs/Security/MixedContent";

this isn't pertinent to the issue at hand because it's in the existing code, but why is the .GetURL function capitalized? I'm wondering if that was a copy-pasta error, since some exports in other similar files are classes. That brings up the issue... can errordoc.js really be classified as an actor? It seems like it's working differently from all of the other actors in that directory. But that's not related to this issue, so should not block it.

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark [:linclark] from comment #14)
> Comment on attachment 8764058 [details]
> Bug 1279195 - Move error message URL onto server;
> 
> https://reviewboard.mozilla.org/r/60104/#review57080
> 
> Tested using this test page: https://www.bennish.net/mixed-content.html.
> Worked as expected. It looks like there are some ESLint issues, so just make
> sure it's green on try before committing.
> 
> Just one minor note below, feel free to fix or not fix as you choose.
> 
> ::: devtools/server/actors/errordocs.js:52
> (Diff revision 1)
> >    JSMSG_UNTERMINATED_STRING: "Unterminated_string_literal",
> >    JSMSG_NOT_CONSTRUCTOR: "Not_a_constructor",
> >    JSMSG_CURLY_AFTER_LIST: "Missing_curly_after_property_list",
> >  };
> >  
> > -exports.GetURL = (errorName) => {
> > +const MIXED_CONTENT_LEARN_MORE = "https://developer.mozilla.org/docs/Security/MixedContent";
> 
> this isn't pertinent to the issue at hand because it's in the existing code,
> but why is the .GetURL function capitalized? I'm wondering if that was a
> copy-pasta error, since some exports in other similar files are classes.
> That brings up the issue... can errordoc.js really be classified as an
> actor? It seems like it's working differently from all of the other actors
> in that directory. But that's not related to this issue, so should not block
> it.

You are right, it's not an actor and should move out of the actors folder.  Should do it in another patch as I imagine we'll want to uplift these.  And yeah, the casing on GetURL should change to fit in with other modules (could be done at the same time as the file move)

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8764233 [details] [diff] [review]
bug1279195.patch

Review of attachment 8764233 [details] [diff] [review]:
-----------------------------------------------------------------

Try push, which I expect to fail on tests like browser_webconsole_allow_mixedcontent_securityerrors.js (see Comment 13): https://treeherder.mozilla.org/#/jobs?repo=try&revision=36eb6b2f8215.

Also, https://developer.mozilla.org/en-US/docs/Web/Security/MixedContent seems to 404

Comment 17

[Florian Scholz (Open Web Docs)]

Attachment: Bug 1279195 - Update error doc URLs and add GA parameters; r=bgrins

Patch with test fixes.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8764551 [details] [diff] [review]
Bug 1279195 - Update error doc URLs and add GA parameters; r=bgrins

Review of attachment 8764551 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/webconsole/test/browser_webconsole_allow_mixedcontent_securityerrors.js
@@ +16,5 @@
>  const TEST_URI = "https://example.com/browser/devtools/client/webconsole/" +
>                   "test/test-mixedcontent-securityerrors.html";
> +const LEARN_MORE_URI = "https://developer.mozilla.org/docs/Web/Security/" +
> +                       "Mixed_content";
> +const GA_PARAMS = "?utm_source=mozilla" +

Please move this into webconsole/test/head.js as 

const DOCS_GA_PARAMS = "?utm_source=mozilla&utm_medium=firefox-console-errors&utm_campaign=default";

or similar.  Then it can be accessed from all of these tests and changed in one place

@@ +67,5 @@
>  function testClickOpenNewTab(hud, results) {
>    let warningNode = results[0].clickableElements[0];
>    ok(warningNode, "link element");
>    ok(warningNode.classList.contains("learn-more-link"), "link class name");
> +  return simulateMessageLinkClick(warningNode, LEARN_MORE_URI + GA_PARAMS);

Nit: I'd rather have the LEARN_MORE_URI already have the params appended instead of adding it in the assertion (in all the tests)

Comment 19

[Florian Scholz (Open Web Docs)]

Attachment: Bug 1279195 - Update error doc URLs and add GA parameters; r=bgrins

Comment 20

[Florian Scholz (Open Web Docs)]

Comment on attachment 8764872 [details] [diff] [review]
Bug 1279195 - Update error doc URLs and add GA parameters; r=bgrins

browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js is failing for me locally, but I couldn't figure it out. Seems like it wasn't failing in your try run.

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Florian Scholz [:fscholz] (MDN) from comment #20)
> browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js is
> failing for me locally, but I couldn't figure it out. Seems like it wasn't
> failing in your try run.

Looks like that test is skipped due to bug Bug 1110500, so it's probably just not running on try but is locally *if* you manually run it i.e. ./mach test browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js.

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8764872 [details] [diff] [review]
Bug 1279195 - Update error doc URLs and add GA parameters; r=bgrins

Review of attachment 8764872 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me, thanks

Comment 23

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df1800c8a548

Comment 24

[Florian Scholz (Open Web Docs)]

Try run looks good to me. Can we land this?

Comment 25

[Brian Grinstead [:bgrins]]

Yeah, looks good to me

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/47fcd8f7257d
Move error message URL onto server. r=linclark
https://hg.mozilla.org/integration/fx-team/rev/0996b504bbbd
Update error doc URLs and add GA parameters; r=bgrins

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47fcd8f7257d
https://hg.mozilla.org/mozilla-central/rev/0996b504bbbd

Comment 28

[Florian Scholz (Open Web Docs)]

The error messages [Learn more] feature is implemented in Firefox 49. This patch,

1) Improves the URL of the error messages, so that MDN auto-detects the locale.
2) Updates several links to MDN so that we avoid unnecessary redirects.
3) Adds Google Analytics parameters to all these URLs, allowing the MDN team to measure success here.

I would like to uplift this into 49. I believe the risks are low and there will still be some testing time in DevEdition and Beta.