875396 - [MMS] Clicking an image in a failed message should open the image, not offer resend

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Corey Frang [:gnarf]]

If you click on the image in a message that has failed to send, it will still open the MozActivity - the resending should be the only thing happening there.

Comment 1

[Corey Frang [:gnarf]]

Attachment: patch v1

Comment 2

[Corey Frang [:gnarf]]

This patch implements it such that we can't click on the image to open, just resend the message...

Not sure which action should win here, open the image, or resend the message.  CC Ayman

Comment 3

[ayman maat :maat]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #2)
> This patch implements it such that we can't click on the image to open, just
> resend the message...
> 
> Not sure which action should win here, open the image, or resend the
> message.  CC Ayman

Hey corey

Ideally we would have two actions associated to the message bubble each initiated depending on where the user clicks: 

1) if the user select the error icon they should get an overlay detailing that the message failed to send and offering the option to resend. this is covered in page 35 and 37 of wireframe pack V8
2) if the user selects the image they open in a failed message they open the image.

is this not possible?

Comment 4

[Corey Frang [:gnarf]]

So, I guessed wrong, coming back with a new patch here soon - thanks for the clarification ayman

Comment 5

[Corey Frang [:gnarf]]

Attachment: patch v2

Okay - this now allows you to view the attachment, but if the attachment is found, it doesn't offer the resend menu.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 753417 [details] [diff] [review]
patch v2

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

r=me with the nits

::: apps/sms/js/thread_ui.js
@@ +23,5 @@
>      console.error('error with open activity', this.error.name);
>      // TODO: Add an alert here with a string saying something like
>      // "There is no application available to open this file type"
>    };
> +  return true;

nit: |return false| for the other return

::: apps/sms/test/unit/thread_ui_test.js
@@ +1172,2 @@
>        messageContainer.appendChild(output);
> +      sinon.stub(ThreadUI, 'handleMessageClick');

nit: if you use this.sinon instead of sinon, the restore is done automatically by the test agent. (this.sinon is a sandbox)

(but test that the tests pass if you change this :) )

@@ +1188,5 @@
>        assert.equal(call.data.blob, testImageBlob);
>      });
> +    test('Does not call handleMessageClick', function() {
> +      assert.isFalse(ThreadUI.handleMessageClick.called);
> +    });

nit: please add some spaces to this part, between tests and setup and teardown

Comment 7

[Corey Frang [:gnarf]]

master: 1ea9f4ae7f3fa7090d7985f6050eebb4d26b0d02

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 410e137