928330 - [DSDS][Gaia] Notify user to switch to subscription to retrieve the MMS from non-active subscription.

Status: RESOLVED FIXED

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

Description

[Ken Chang[:kenkai]]

If user receives a subscription#2 MMS message, while SIM/data is active on subscription#1, Gaia needs to notify user to switch to subscription#2 to retrieve the MMS. (either "silently" or manually, per user's preference in settings)

Comment 1

[Ken Chang[:kenkai]]

Joe, This needs help from Comms app.

Comment 2

[Kai-Chih Hu [:khu]]

It looks like 1.3+ to me.

Comment 3

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

change component so it shows up in comms queries

Comment 4

[Julien Wajsberg [:julienw]]

I don't understand this bug.

if the MMS is received through subscription 2, then the MMS should be downloaded through subscription 2.

Why Gaia does need to know this ?

The API is "mozMobileMessage.retrieveMMS(id)". Therefore Gecko should have all the necessary information to do this properly.

Comment 5

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> I don't understand this bug.
> 
> if the MMS is received through subscription 2, then the MMS should be
> downloaded through subscription 2.
> 
> Why Gaia does need to know this ?
> 
> The API is "mozMobileMessage.retrieveMMS(id)". Therefore Gecko should have
> all the necessary information to do this properly.

Maybe I'm not the proper candidate to answer this question, please correct me if my brief comment is wrong:

Yes, Gecko might be able to switch the connection and handling the message retrival by their own, Here is a situation is: Subscription#1 data connection is activated and subscription#2 set mms auto retrieve. When #2 need to retrieve mms, #2 connection should be activated, but only one SIM/data could be activate at the time, which means #1 should be deactivate if we want to retrieve mms for #2. But this will cause data connection closed(because #1 is still the preferable SIM). User can choose whether they want to switch the connection for mms retrieval now or later until user config their settings.

Comment 6

[Julien Wajsberg [:julienw]]

We have the same behaviour currently when we are on 2G (as opposed to 3G): when using the telephony API (dialing or SMS), the data is closed. AFAIK we don't show any message to the user in this case...

But now I understand the issue, not sure what is the ideal solution here...

Comment 8

[Kai-Chih Hu [:khu]]

Do we know the possible target milestone for this bug? Thanks.

Comment 9

[Kai-Chih Hu [:khu]]

Steve, I will just assign the target milestone to sprint 6 for now. Please move it to the accurate milestone that you're seeing and expecting to land it. Thanks!

Comment 10

[Steve Chung [:steveck]]

I'm ok with Sprint 5, thanks.

Comment 11

[Kai-Chih Hu [:khu]]

(In reply to Steve Chung [:steveck] from comment #10)
> I'm ok with Sprint 5, thanks.

Awesome!!! Thanks, Steve.

Comment 12

[Steve Chung [:steveck]]

Attachment: Link to github

Since I haven't test it on DSDS device(and I'm not sure when I could get one...), I'll not set review request before testing on device. But any feedback is welcome for sure:)

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 832832 [details] [review]
Link to github

feedback given!

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 832832 [details] [review]
Link to github

Basically, I think we should rewrite the showMessageError function using my suggestion in [1], and that the "leftSide" part of code should be removed because UX designs must not be taken literally :)

[1] https://github.com/mozilla-b2g/gaia/pull/13064#issuecomment-27877748

Comment 15

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Comment on attachment 832832 [details] [review]
> Link to github
> 
> Basically, I think we should rewrite the showMessageError function using my
> suggestion in [1], and that the "leftSide" part of code should be removed
> because UX designs must not be taken literally :)
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/13064#issuecomment-27877748

I've updated the patch with your suggestion(but a little bid different). I think the the show error dialog option could be more general for most of the case. Wdyt?

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 832832 [details] [review]
Link to github

will try to check today

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 832832 [details] [review]
Link to github

Oki, I left some more comments on github.

I especially would like to know Gene and Carrie's advice about [1].

Thanks!

[1] https://groups.google.com/forum/#!topic/mozilla.dev.webapi/2FbqKiVYufE

Comment 19

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Comment on attachment 832832 [details] [review]
> Link to github
> 
> Oki, I left some more comments on github.
> 
> I especially would like to know Gene and Carrie's advice about [1].
> 
> Thanks!
> 
> [1] https://groups.google.com/forum/#!topic/mozilla.dev.webapi/2FbqKiVYufE

Ya, we all read through the thread and Gene will reply in the web-api mail-list later. The API and behavior should be confirmed. I think Gene will have a clear answer in the mail, but I just quick update here:

We "must" switch the data connection for downloading the message. Data and MMS could not exist in different SIM. Even Android DSDS device will disconnect original data connection, the SIM switch processed automaticly with warning dialog that nofity user currect data connection will be closed. Original data connection will establish again while MMS retrival finished. We could propose re-connect mechanism in the future version, but SIM switch and warning dialog is needed for sure in this case.

Comment 20

[Kai-Chih Hu [:khu]]

Need verification on real devices.

Comment 21

[Steve Chung [:steveck]]

Comment on attachment 832832 [details] [review]
Link to github

Hi Juien, I update the default SIM id to settings part, may need you feedback for this changes. About the dialog, the reason why I move the logic outside the showMessage/error is because I want to make the error dialog interface more general, we don't need more flags for other type of error(like non-activate sim error). Do you still prefer your original idea?

Comment 22

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #21)
> Comment on attachment 832832 [details] [review]
> Link to github
> 
> Hi Juien, I update the default SIM id to settings part, may need you
> feedback for this changes. About the dialog, the reason why I move the logic
> outside the showMessage/error is because I want to make the error dialog
> interface more general, we don't need more flags for other type of
> error(like non-activate sim error). Do you still prefer your original idea?

I think you're getting constrained because showMessageError is only one function. We could definitely have 2 functions: showMessageError like it is now, and handleMessageError that would do what I asked to move to showMessageError.

And I think it would make more sense to move this to an external component, and that would also probably help you think in a more generic way.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 832832 [details] [review]
Link to github

feedback given on github

Comment 24

[Steve Chung [:steveck]]

Comment on attachment 832832 [details] [review]
Link to github

I moved the error dialog into dialog.js(error dialog inherits from dialog) and  add other fixing from settings related module. I'll add test ASAP once you agree the changes(Or you may want to move the error dialog into a new file?).

Comment 25

[Julien Wajsberg [:julienw]]

I think it's fine to keep it in dialog.js, thanks!
Will have a look next week :)

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 832832 [details] [review]
Link to github

Hi Julien, I've updated the patch for test cases and jshint error fixing, so I changed flag to review instead of feedback. Thanks for the feedbacks previously!

Comment 27

[Julien Wajsberg [:julienw]]

Comment on attachment 832832 [details] [review]
Link to github

This is already a solid patch, thanks!

I still have a big architecture requirement, and various small comments.

Please add your follow-ups in a separate commit :)
Thanks!

Comment 28

[Steve Chung [:steveck]]

Comment on attachment 832832 [details] [review]
Link to github

Hi Julien, I made some changes based on your suggetion. I'll also add some comment on the github for more description, thanks.

Comment 29

[Kai-Chih Hu [:khu]]

Good job, Steve. 
So, it looks like this one could be landed tomorrow? :)

Comment 30

[Julien Wajsberg [:julienw]]

Probably yes ;)

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 832832 [details] [review]
Link to github

r=me with the test changes

please file the follow-up bug for the displaying the recipients in error messages, to replace <br> with proper lists.

Comment 32

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> Comment on attachment 832832 [details] [review]
> Link to github
> 
> r=me with the test changes
> 
> please file the follow-up bug for the displaying the recipients in error
> messages, to replace <br> with proper lists.

Thanks for the review! I've created a follow-up Bug 944644 for the issue you mentioned.

Comment 33

[Steve Chung [:steveck]]

Merged in master:  8eb10161896285c03342aafd051222496f1090e8