981582 - [Messages] Resend option from bug 933365 should not be added in the contextmenu for incoming messages

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

STR:
* turn auto retrieve off
* receive a MMS with one attachment
* turn airplane mode
* tap "download" to download the MMS
=> the "error" icon is displayed
* long press the MMS bubble

Expected:
* we should not see the "resend" option

Actual:
* we see the "resend" option

Comment 1

[Gabriele Svelto [:gsvelto]]

I think this is caused by the way we select the elements that have the resend option, we just check for the error class to be present so this will affect all error'd messages:

https://github.com/mozilla-b2g/gaia/blob/28497d16c9f5d3feafaf3ca5b45cabd2df157366/apps/sms/js/thread_ui.js#L1997

Comment 2

[Julien Wajsberg [:julienw]]

Yep definitely this :)

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

is it a regression from bug 933365 ?

Comment 4

[Julien Wajsberg [:julienw]]

Yep, that's why I added it in the "blocks" line.

Comment 5

[Jason Smith [:jsmith]]

This is a great idea to clean up, but I don't think it's a blocker. Feel free to fix this though - it's a good thing to clean up.

Comment 6

[Julien Wajsberg [:julienw]]

Adding the mentored flag.

The piece of code to change is at [1], you need to check if it has the class "outgoing" as well.

[1] https://github.com/mozilla-b2g/gaia/blob/a8c1a5ad0936cf5c131afaf2bb5cf6b068e8535d/apps/sms/js/thread_ui.js#L2023-L2036

Comment 7

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: https:::github.com:mozilla-b2g:gaia:pull:17531

Ask for mentor review

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8395705 [details]
https:::github.com:mozilla-b2g:gaia:pull:17531

I commented on the PR

Please request review when you did the changes :)

thanks a lot!

Note: you can ask the next review from Steve Chung if it's easier for you!

Comment 9

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #8)
> Comment on attachment 8395705 [details]
> https:::github.com:mozilla-b2g:gaia:pull:17531
> 
> I commented on the PR
> 
> Please request review when you did the changes :)
> 
> thanks a lot!
> 
> Note: you can ask the next review from Steve Chung if it's easier for you!

Thanks, I will modify accordingly and ask for review again!

Comment 10

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Steve -

Per Comment#8, Julien suggest me to seek help from you as a mentor to review my patch. I just update again my pull request based on Julien's feedbacks, could you kindly help to review my patch again? Sorry in advance that I am not familiar with the unit test framework so i probably made several mistakes....

Thanks

Vance

Comment 11

[Julien Wajsberg [:julienw]]

Moving the mentor flag to Steve, it's certainly easier since you're colocated.
Steve, if your're too busy don't hesitate to redirect it to me :)

Comment 12

[Steve Chung [:steveck]]

Hi Vance, I add some feedback for test on the github. Just ping me if you are still struggling for the tests, thanks!

Comment 13

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Steve Chung [:steveck] from comment #12)
> Hi Vance, I add some feedback for test on the github. Just ping me if you
> are still struggling for the tests, thanks!

Hi Steve, I raise some questions in the github, please help to check when you have time!!!

Comment 14

[Steve Chung [:steveck]]

Hi Vance, I replied on github: https://github.com/mozilla-b2g/gaia/pull/17531/files#r11009169

Comment 15

[Julien Wajsberg [:julienw]]

Hey Vance, I see you closed the PR, do you plan to open a new one? (maybe you already did ? ;) )

Also do you want to finish the bug yourself or would you rather see it finished by someone else?

Comment 16

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> Hey Vance, I see you closed the PR, do you plan to open a new one? (maybe
> you already did ? ;) )
> 
> Also do you want to finish the bug yourself or would you rather see it
> finished by someone else?

Hi Julien, I kinda mess up my previous PR while doing the rebase and squash, so yes I am preparing another PR now :)

Comment 17

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18442/files

Finally complete the unit test, also the Travis is all green, please help to review again!

Comment 18

[Steve Chung [:steveck]]

Some comments in https://github.com/mozilla-b2g/gaia/pull/18442

Comment 19

[Steve Chung [:steveck]]

Hey Vance, please ping me when your patch is ready, thnaks!

Comment 20

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Steve Chung [:steveck] from comment #19)
> Hey Vance, please ping me when your patch is ready, thnaks!

Done again, new pull request here, already modified based on your comments, and sorry I need to re-submit a new one since everytime i try to re-base, I end up screwing up my code base

https://github.com/mozilla-b2g/gaia/pull/18663

Thanks

Vance

Comment 21

[Steve Chung [:steveck]]

Comment on attachment 8408766 [details] [diff] [review]
https://github.com/mozilla-b2g/gaia/pull/18442/files

Hey Vance , next time you can paste 'https://github.com/mozilla-b2g/gaia/pull/18442' to the input field and bugzilla will convert it into validate link.

I still leave some comments in your new pr, seems you got some indention problem in the patch. Please attach your new pr in the attachement so that we can review it, thanks.

Comment 22

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18663

Hi Steve, new patch is ready, i fix the indent style problem, please help to review and let me know if there is any problem

Comment 23

[Steve Chung [:steveck]]

Almost there except 1 indention nit(and squash comments). Could you please submit the rebased patch for the final round? thanks!

Comment 24

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Steve Chung [:steveck](PTO until 5/12) from comment #23)
> Almost there except 1 indention nit(and squash comments). Could you please
> submit the rebased patch for the final round? thanks!

Sure~ attached please find the newest rebase and squash patch

Comment 25

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18663 r=julien

Comment 26

[Julien Wajsberg [:julienw]]

I'll take this because Steve is away

Comment 27

[Julien Wajsberg [:julienw]]

Sorry, still a nit, but should be fine after this.

Please needinfo me again or ask review on the attachment when you're ready.

No need to add a new commit, just amend the current one !

Comment 28

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #27)
> Sorry, still a nit, but should be fine after this.
> 
> Please needinfo me again or ask review on the attachment when you're ready.
> 
> No need to add a new commit, just amend the current one !

Amend done, please help to review again~

Thanks

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 8413531 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18663 r=julien

r=me, let's move forward, thanks !

You need to add "r=julien" at the end of the first line in the commit log (use git commit --amend), then push to your pull request, then, if you don't have the commit rights, add "checkin-needed" in the keywords field, otherwise you're free to commit yourself.

Comment 30

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #29)
> Comment on attachment 8413531 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/18663
> 
> r=me, let's move forward, thanks !
> 
> You need to add "r=julien" at the end of the first line in the commit log
> (use git commit --amend), then push to your pull request, then, if you don't
> have the commit rights, add "checkin-needed" in the keywords field,
> otherwise you're free to commit yourself.

Commit message amend done, also add checkin-needed keyword since i don't have commit right. Thanks!

Comment 31

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18663

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/0a3636f5df98bdc3174fdfe80cce1681755f5b8f