663085 - No user notification when XPI download or test fails

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Add-on Builder, defect, P3)

Description

[Ryan N. Freebern [:rfreebern]]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

After clicking the XPI download link, the code makes 50 attempts to download the newly-built XPI. If it reaches the 50-attempt mark without successfully downloading, it quits silently with no notification to the user.

Reproducible: Always

Steps to Reproduce:
1. Add "return HttpResponse('{"ready": false}')" at the top of check_download in apps/xpi/views.py so that it never indicates that the download is ready.
2. Click the download button on an add-on edit screen.
3. Wait while all 50 attempts complete without success.

Actual Results:  
After 50 attempts, the spinner disappears from the download button but no other notification is given that the download process failed.

Expected Results:  
If 50 download attempts do not result in starting a download, the user should receive a notification that the download failed and they should try again.

Comment 1

[Piotr Zalewa [:zalun]]

I'd like to add more to this. We already agreed that we will display precise SDK stderr if ``cfx xpi`` will result with error.

In such case we could create an error file {hashtag}.error instead of {hashtag}.xpi and return a HttpResponseFailed. JS will then create another request to get the error from the {hashtag}.error file. Currently test if file was created or not was simply - empty response - XPI not created yet, something in - we've got XPI.

Comment 2

[Wil Clouser [:clouserw]]

> In such case we could create an error file {hashtag}.error instead of
> {hashtag}.xpi and return a HttpResponseFailed. JS will then create another
> request to get the error from the {hashtag}.error file. Currently test if
> file was created or not was simply - empty response - XPI not created yet,
> something in - we've got XPI.

No need for a separate request.  Instead of {"ready": false} we can just return {"ready": false, "msg": "..."}.  I don't think that is the same bug as comment 0 though.

Comment 3

[Piotr Zalewa [:zalun]]

next request is not needed indeed however not like you suggesting as {"ready": false} is returned before the xpi is created. It will be enough to return the error and in the content provide the error text

Comment 4

[Ryan N. Freebern [:rfreebern]]

@zalun You took care of displaying stderr, right?

I've written code to address the problem in comment 0:

https://github.com/rfreebern/FlightDeck/commit/c83238e6784a400e4d69f2f693e13c84586e1803

Comment 5

[Piotr Zalewa [:zalun]]

Yes - but I implemented it to the Test feature only. It reads json.message instead, we need to consider if displaying a precise error would be better.

Comment 6

[Daniel Buchner [:dbuc]]

(In reply to Piotr Zalewa [:zalun] from comment #5)
> Yes - but I implemented it to the Test feature only. It reads json.message
> instead, we need to consider if displaying a precise error would be better.

Does this fix need to be implemented elsewhere? If so, where?

Comment 7

[Piotr Zalewa [:zalun]]

https://github.com/mozilla/FlightDeck/pull/84