Don't register a visit if page returns a non-successful HTTP status code #4032
Conversation
| @@ -965,6 +965,11 @@ class Frame extends ImmutableComponent { | |||
| })) | |||
| } | |||
| }) | |||
| this.webview.addEventListener('did-get-response-details', (details) => { | |||
| if (getSetting(settings.PAYMENTS_ENABLED) && details.originalURL === this.props.location) { | |||
There was a problem hiding this comment.
I think there are other places we want this so it should always fire regardless of ledger settings. Also, we don't really need to send frameProps for all window actions now that we have tabId so I think you only only need to send the tabId and the details
| @@ -128,6 +139,9 @@ const doAction = (action) => { | |||
| // retains all past pages, not really sure that's needed... [MTR] | |||
| eventState = eventState.set('page_info', eventState.get('page_info').push(action.pageInfo)) | |||
| break | |||
| case WindowConstants.WINDOW_GOT_RESPONSE_DETAILS: | |||
| addResponseDetails(action.frameProps.get('location'), action.frameProps.get('tabId'), action.details) | |||
There was a problem hiding this comment.
url is also in the response details
There was a problem hiding this comment.
as we discussed, I think we should just use this for the load event and add the details to it
| @@ -17,7 +17,8 @@ const BrowserWindow = electron.BrowserWindow | |||
| let eventState = Immutable.fromJS({ | |||
| page_load: [], | |||
| page_view: [], | |||
| page_info: [] | |||
| page_info: [], | |||
| page_response: [] | |||
There was a problem hiding this comment.
maybe page_load_details?
| * @param {number} responseCode - HTTP response code to be evaluated | ||
| * @return {boolean} true if the page should be excluded, false if not | ||
| */ | ||
| module.exports.IsValidResponseCode = (responseCode) => { |
There was a problem hiding this comment.
IsValidResponseCode => isValidResponseCode
| * @param {number} responseCode - HTTP response code to be evaluated | ||
| * @return {boolean} true if the page should be excluded, false if not | ||
| */ | ||
| module.exports.IsValidResponseCode = (responseCode) => { |
There was a problem hiding this comment.
Feedback about the method for checking (and what codes to include) is very much appreciated
There was a problem hiding this comment.
I think you probably want to check for errors vs positive responses because there are other potential successful responses. You also want to ignore things like redirects
There was a problem hiding this comment.
actually I take that back. We need to further restrict 2xx to 200, 203 and 206 I think
There was a problem hiding this comment.
this also might be better as either a general utility method vs ledger. shouldTrackView looks ledger-specific so that's ok
There was a problem hiding this comment.
Good call- typically REST APIs will use 201 and 202, which shouldn't be returned for regular content 👍
| @@ -965,6 +965,11 @@ class Frame extends ImmutableComponent { | |||
| })) | |||
| } | |||
| }) | |||
| this.webview.addEventListener('did-get-response-details', (details) => { | |||
| if (getSetting(settings.PAYMENTS_ENABLED) && details.originalURL === this.props.location) { | |||
There was a problem hiding this comment.
NOTE: this event will only get sent if payments is enabled (to prevent unneeded overhead)
Fixes #3759 Auditors: @mrose17, @bridiver Test Plan: 1. Visit https://clifton.io 2. Stay on page for a while 3. Confirm time spent is incremented 4. Visit https://clifton.io/404 5. Stay on page for a while 6. Confirm time spent is not incremented Udpated w/ review feedback from @bridiver
Auditors: @bridiver Test Plan:
| it('returns false for non-content success codes (used for REST apis, etc)', function () { | ||
| assert.equal(httpUtil.responseHasContent(201), false) // created | ||
| assert.equal(httpUtil.responseHasContent(202), false) // accepted | ||
| }) |
There was a problem hiding this comment.
4xx? Don't necessarily need an exhaustive list, but the list isn't that long https://en.wikipedia.org/wiki/List_of_HTTP_status_codes
can be done in a followup too
|
++ |
|
Did not work as expected for 0.12.2 RC2 on OS X |
git rebase -ito squash commits (if needed).Don't register a visit if page returns a non-successful HTTP status code
Fixes #3759
Auditors:
@mrose17, @bridiver
Test Plan: