811159 - Save last uploaded payload

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Save payload after upload, v1

One of the requirements for about:healthreport is to allow display of the previously-uploaded payload any time. This necessitates the requirement of saving the payload locally.

The attached patch does just that. I'm not asking for full review yet because I just got this working and it could almost certainly use some clean-up. Why submit a patch at all? I don't know either. Long day and I want something to show for it, I suppose.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 680892 [details] [diff] [review]
Save payload after upload, v1

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

I'd prefer to see a test for a failing write.

::: services/healthreport/healthreporter.jsm
@@ +387,5 @@
>                                      payload,
>                                      this.lastSubmitID);
>  
> +    return promise.then(this._onBagheeraResult.bind(this, request, false))
> +                  .then(this._saveLastPayload.bind(this, payload));

I'd argue that we want to save the payload first. We don't want to upload something that we then fail to write out; we won't be able to display it to the user. (And perhaps we want something else to do the upload.)

@@ +448,5 @@
> +
> +    return OS.File.writeAtomic(path, buffer, {tmpPath: pathTmp});
> +  },
> +
> +  getLastPayload: function getLoadPayload() {

JavaDoc, omit function name.

::: services/healthreport/tests/xpcshell/head.js
@@ +2,5 @@
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> +do_get_profile();

I suggest putting a comment here, and a link to Bug 810543.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Save payload before upload, v2

Saving before upload now.

I left the function name in because I think the style should be consistent. And, we haven't yet investigated whether exceptionStr preserves it, right? I figure we can bulk update our style in the future.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 681839 [details] [diff] [review]
Save payload before upload, v2

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

Good enough for me.

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/larch/rev/d873cf1840b6

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 681839 [details] [diff] [review]
Save payload before upload, v2

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).

Comment 6

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e76044ee76e3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e76044ee76e3

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 681839 [details] [diff] [review]
Save payload before upload, v2

FHR for B2G ADU ping, won't be built/enabled for Mobile/Desktop.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06d2da2edb4c
https://hg.mozilla.org/releases/mozilla-beta/rev/95195818bb7d