Closed Bug 1431306 Opened 6 years ago Closed 6 years ago

Clicking on the status code of a 418 response in the console does nothing

Categories

(DevTools :: Console, defect, P2)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: nchevobbe, Assigned: abhinav.koppula, Mentored)

References

Details

Attachments

(1 file)

Bug 1431306 - Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
Steps to reproduce:
1. Open the console
2. Make sure the "XHR" filter is on
3. Evaluate `fetch("https://httpstat.us/418")`
4. Click on the "418" status code


Expected results:

The MDN page to the status code is opened

Actual results:

Nothing happens, despite the fact that there was a "Learn more" tooltip when I hovered the status code

This is because MDN does not have a dedicated page to the 418 code. There are a few other codes that don't have a page as well.
In those case, we should navigate the user to the status code list page: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

---

This is a follow-up of Bug 1421213, we added the ability to navigate to the MDN page of a status by clicking on the status code of a response.
Abhinav, would you be interested working on this ?
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(abhinav.koppula)
Priority: -- → P2
Yes, I can take this up.
Flags: needinfo?(abhinav.koppula)
Thanks a lot Abhinav
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Hi Nicolas,
I've created a quick patch. Do you think a test of mdn-utils makes sense for this which would just test the getHTTPStatusCodeURL method?
Comment on attachment 8944185 [details]
Bug 1431306 - Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page;

https://reviewboard.mozilla.org/r/214470/#review220146

::: devtools/client/netmonitor/src/utils/mdn-utils.js:143
(Diff revision 1)
>      "504",
>      "505",
>      "511"
>  ];
>  
>  const MDN_URL = "https://developer.mozilla.org/docs/";

I think we should remove the en-US part so the user gets redirected to the appropriate localized page

::: devtools/client/netmonitor/src/utils/mdn-utils.js:172
(Diff revision 1)
>    let idx = SUPPORTED_HTTP_CODES.indexOf(statusCode);
>    return idx > -1 ?
>      `${MDN_URL}Web/HTTP/Status/${SUPPORTED_HTTP_CODES[idx] + getGAParams(panelId)}`
> -      : null;
> +      : MDN_STATUS_CODES_LIST_URL + getGAParams(panelId);

Could it be : 
```js
return (
  SUPPORTED_HTTP_CODES.includes(statusCode)
    ? `${MDN_URL}Web/HTTP/Status/${statusCode}`
    : MDN_STATUS_CODES_LIST_URL
  ) + getGAParams(panelId);
```
(In reply to Abhinav Koppula from comment #5)
> Hi Nicolas,
> I've created a quick patch. Do you think a test of mdn-utils makes sense for
> this which would just test the getHTTPStatusCodeURL method?

Yes, it looks like a good idea.
We can have an xpcshell test for this.
Comment on attachment 8944185 [details]
Bug 1431306 - Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page;

Missing tests
Attachment #8944185 - Flags: review?(nchevobbe) → review-
Comment on attachment 8944185 [details]
Bug 1431306 - Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page;

https://reviewboard.mozilla.org/r/214470/#review221180

A couple of nits, but this looks good to me ! Thanks Abhinav.
Let's ask Honza what he thinks about it since we are introducing a new folder.

::: commit-message-6bb6f:1
(Diff revision 2)
> +Bug 1431306 - Clicking on the status code of a 418 response in the console does nothing; r=nchevobbe

nit: The commit message should be about what the patch does, e.g. "Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page."

::: devtools/client/netmonitor/test/unit/test_mdn-utils.js:25
(Diff revision 2)
> +    getNetMonitorTimingsURL,
> +    getPerformanceAnalysisURL
> +  } = require("devtools/client/netmonitor/src/utils/mdn-utils");
> +
> +  info("Checking for supported headers");
> +  ok(getHeadersURL("Accept") === `${MDN_URL}Web/HTTP/Headers/Accept${GTM_PARAMS_NM}`);

nit: could we use `is` instead of `ok` ? It is easier to read an error from `is` since you get something like: 

"Got 'a', expected 'b'"

whereas `ok` will only tell you that it fails, without telling you why
Attachment #8944185 - Flags: review?(nchevobbe) → review+
Comment on attachment 8944185 [details]
Bug 1431306 - Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page;

Honza, can you have a look at this patch and tell us if everything seems okay to you ? Thanks !
Attachment #8944185 - Flags: feedback?(odvarko)
Comment on attachment 8944185 [details]
Bug 1431306 - Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page;

Thanks Abhinav for working on this!

I like the new logic that navigates the user to the status-list page [1] if there is no support for the actual code. But, note that there is MDN page for status 418 [2]

F+, if 418 is added to our list of SUPPORTED_HTTP_CODES

Honza

[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
[2] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/418_I_m_a_teapot
Attachment #8944185 - Flags: feedback?(odvarko) → feedback+
The 418 page was moved to https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/418 so it follow the same rule as the other status url, and is compliant with our implementation :)
Abhinav, could you add the 418 status to the list of supported codes and test an imaginary one in the test (like, 999)
Comment on attachment 8944185 [details]
Bug 1431306 - Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page;

https://reviewboard.mozilla.org/r/214470/#review221180

Thanks for the review, Nicolas. Added 418 also to the list.

Also, there isn't an `is` assertion but there is `equal` assertion which does what we want. 
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

Using equal gives the following meaningful result:
Expected PASS, got FAIL
[run_test : 33] "https://developer.mozilla.org/docs/Web/HTTP/Status?utm_source=mozilla&utm_medium=devtools-webconsole&utm_campaign=default" == "https://developer.mozilla.org/docs/Web/HTTP/Status"
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7158c934d336
Return the MDN status list URL in getHTTPStatusCodeURL when statusCode does not have a dedicated page; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/7158c934d336
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: