Closed Bug 928330 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+

People

(Reporter: kchang, Assigned: steveck)

References

Details

(Whiteboard: [Blocked by devices])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review
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)
Joe, This needs help from Comms app.
Flags: needinfo?(jcheng)
It looks like 1.3+ to me.
blocking-b2g: 1.3? → 1.3+
change component so it shows up in comms queries
Component: Gaia → Gaia::SMS
Flags: needinfo?(jcheng)
Assignee: nobody → schung
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.
(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.
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...
Duplicate of this bug: 927812
Do we know the possible target milestone for this bug? Thanks.
Flags: needinfo?(schung)
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!
Target Milestone: --- → 1.3 Sprint 6 - 12/6
I'm ok with Sprint 5, thanks.
Flags: needinfo?(schung)
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 Sprint 5 - 11/22
(In reply to Steve Chung [:steveck] from comment #10)
> I'm ok with Sprint 5, thanks.

Awesome!!! Thanks, Steve.
Attached file 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:)
Attachment #832832 - Flags: feedback?(felash)
Comment on attachment 832832 [details] [review]
Link to github

feedback given!
Attachment #832832 - Flags: feedback?(felash)
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
(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 on attachment 832832 [details] [review]
Link to github

will try to check today
Attachment #832832 - Flags: feedback?(felash)
Duplicate of this bug: 917954
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
Attachment #832832 - Flags: feedback?(felash)
(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.
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Need verification on real devices.
Whiteboard: [Blocked by devices]
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?
Attachment #832832 - Flags: feedback?(felash)
(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 on attachment 832832 [details] [review]
Link to github

feedback given on github
Attachment #832832 - Flags: feedback?(felash)
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?).
Attachment #832832 - Flags: feedback?(felash)
I think it's fine to keep it in dialog.js, thanks!
Will have a look next week :)
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!
Attachment #832832 - Flags: feedback?(felash) → review?(felash)
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!
Attachment #832832 - Flags: review?(felash)
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.
Attachment #832832 - Flags: review?(felash)
Good job, Steve. 
So, it looks like this one could be landed tomorrow? :)
Probably yes ;)
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.
Attachment #832832 - Flags: review?(felash) → review+
Blocks: 944644
(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.
Merged in master:  8eb10161896285c03342aafd051222496f1090e8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 945649
Depends on: 949326
Duplicate of this bug: 897925
You need to log in before you can comment on or make changes to this bug.