Closed Bug 1022575 Opened 6 years ago Closed 6 years ago

Received SMS store (without text) on check balance.

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

VERIFIED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: lolimartinezcr, Assigned: azasypkin)

References

Details

(Keywords: regression, Whiteboard: [p=2])

Attachments

(3 files, 1 obsolete file)

Attached image 2014-06-09-11-41-40.png
Tested
2.0
Hamachi
Reproducible 100%
Platform versión: 32.0a1
Build ID: 20140609083901
Git commit: deed49c5

Pre-requisites: Sim card with billing support.

STRs:
1. Tap usage application.
2. Tap "Update" button.
3. Tap message application.

Actual result:
You can see thread with messages about cost control application and when user taps in a message, it is empty (without text). See attached image.

Expected result:
Any messages about cost control is received.
Attached image 2014-06-09-11-42-01.png
These threads disappear when the sms app is closed and reopened. I don't know how to force a refresh of the thread after a sms deletion.

Julien, can you help us with this?
Regards
Flags: needinfo?(felash)
I can think of several things:

* ignore the blacklisted SMS when loading the messages at all
* have a "ondeleted" event in [1] so that we can react on it

I'd do the first option, seems easier.

But first, Loli, I'd like to know if the issue happens on previous versions as well?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
Flags: needinfo?(felash) → needinfo?(lolimartinezcr)
(In reply to Julien Wajsberg [:julienw] from comment #3)
> I can think of several things:
> 
> * ignore the blacklisted SMS when loading the messages at all
> * have a "ondeleted" event in [1] so that we can react on it
> 
> I'd do the first option, seems easier.
> 
> But first, Loli, I'd like to know if the issue happens on previous versions
> as well?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/
> nsIDOMMobileMessageManager.idl

Im not sure about what you want to know, because it is new feature.
Flags: needinfo?(lolimartinezcr)
(In reply to Loli from comment #4)
> (In reply to Julien Wajsberg [:julienw] from comment #3)
> > I can think of several things:
> > 
> > * ignore the blacklisted SMS when loading the messages at all
> > * have a "ondeleted" event in [1] so that we can react on it
> > 
> > I'd do the first option, seems easier.
> > 
> > But first, Loli, I'd like to know if the issue happens on previous versions
> > as well?
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/
> > nsIDOMMobileMessageManager.idl
> 
> Im not sure about what you want to know, because it is new feature. This bug has been created when I tested Bug 987156 - [User Story] Cost Control: Do not store SMS sent or received on check balance and top-up
Hey Bevis,

see comment 3.

Is it possible to have a "deleted" event for v2.0?

All other solutions we discussed with Marina feels both complicated and hacky...
Flags: needinfo?(btseng)
(In reply to Julien Wajsberg [:julienw] from comment #6)
> Hey Bevis,
> 
> see comment 3.
> 
> Is it possible to have a "deleted" event for v2.0?
> 
> All other solutions we discussed with Marina feels both complicated and
> hacky...

Hi Julien,

I'd like to double confirm if the received messages are the messages sent from SilentNumber. [1]
If yes, we didn't see any deleting-related operations from gecko because the message is actually created and notify directly to gaia without access to DB (save and delete).

Is there anything abnormal in SMS app instead?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#2955
Flags: needinfo?(btseng)
Bevis, how a "silent message" is created ? I don't know much about this to be honest.

I don't think that's what's currently used, but maybe we should.
Flags: needinfo?(btseng)
Hi,
the sms are deleted by usage application. The problem is not related with the silent mode.

The str is the following:
 1. SMS app is opened in background
 2. Usage app deletes the sms
 3. Open the sms app. Then the threads of the deleted sms are showed with the last text received, because the sms app doesn't know that this sms are deleted. When the user click on the sms, the message is empty, because was deleted by usage app.

For this reason, if a sms-deleted event is launched, the sms app can detect when it's necessary a ui refresh to avoid the current behavior

If the sms app is closed after the deleting this behaviour does not occurs.
Marina: but maybe the SMS to update the balance should be sent using sendSilentSms instead of sendSms ? (note: I don't know the actual name of the method)
AFAIK this method of the API is not available from gaia, only can used from gecko for security reasons.
(In reply to Julien Wajsberg [:julienw] from comment #8)
> Bevis, how a "silent message" is created ? I don't know much about this to
> be honest.
> 
> I don't think that's what's currently used, but maybe we should.

After further study, as marina mentioned, this API can only be used from gecko in b2g/chrome/content/payment.js for security reasons.
Instead of defining new event in a rush, for the scenario in comment 9, 
is it possible to refresh the UI of the SMS app when it resumed from background to foreground?
Flags: needinfo?(btseng) → needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #10)
> Marina: but maybe the SMS to update the balance should be sent using
> sendSilentSms instead of sendSms ? (note: I don't know the actual name of
> the method)

As Marina mentioned, the silent SMS flag is not exposed to web content. It is only used internally for payments [1] and the new MobileID API [2]. We had some discussions about this in the past and we figured that all the use cases could be fulfilled with the current WebSMS API and a collaboration with the Gaia SMS app.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=816564
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=988469

(In reply to marina rodríguez [:mai] from comment #9)

> Hi,
> the sms are deleted by usage application. The problem is not related with
> the silent mode.
> 
> The str is the following:
>  1. SMS app is opened in background
>  2. Usage app deletes the sms
>  3. Open the sms app. Then the threads of the deleted sms are showed with
> the last text received, because the sms app doesn't know that this sms are
> deleted. When the user click on the sms, the message is empty, because was
> deleted by usage app.
> 
> For this reason, if a sms-deleted event is launched, the sms app can detect
> when it's necessary a ui refresh to avoid the current behavior
> 
> If the sms app is closed after the deleting this behaviour does not occurs.

I am not saying that an event like this is not needed in the SMS API, but a way to workaround this is to notify from Cost Control to the SMS app via IAC about the SMS deletion.
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #12)
> (In reply to Julien Wajsberg [:julienw] from comment #8)
> > Bevis, how a "silent message" is created ? I don't know much about this to
> > be honest.
> > 
> > I don't think that's what's currently used, but maybe we should.
> 
> After further study, as marina mentioned, this API can only be used from
> gecko in b2g/chrome/content/payment.js for security reasons.
> Instead of defining new event in a rush, for the scenario in comment 9, 
> is it possible to refresh the UI of the SMS app when it resumed from
> background to foreground?

I would prefer to avoid wasting any resources refreshing and repainting if nothing changed in the SMS DB. We need a way to tell the SMS app that something changed. We can do that from the API if we expect 3rd party consumers of the WebSMS API (something like the oncontactchange in the Contacts API) or from the known responsibles of these changes in the SMS DB, which if I am not wrong is only the Cost Control app so far.

Another alternative to the WebSMS SMS deletion event or the IAC workaround mentioned in comment 13 would be to expose a property with the version of the MobileMessage DB. With this property, the SMS app would be able to know if something changed since its last DB query and so it would be able to react accordingly. Note that this property would have the extra benefit of allowing the SMS app to handle local caches if needed, but that's another topic.
> Instead of defining new event in a rush, for the scenario in comment 9, 

Really, it's not that much, is it?

> is it possible to refresh the UI of the SMS app when it resumed from background to foreground?

That would mean fetching all messages and rendering again. Then no :)

> Another alternative would be to expose a property with the version of the MobileMessage DB. 

Not sure it's simpler :p
I don't think we should do this, especially in light of future Datastore work that will give this for free.


The easiest is to exactly know which message has been deleted so that we can update the ui. IAC would work (could be an argument to the "disable" operation) but looks overkill.
Flags: needinfo?(felash)
Filed bug 1022575 to address the possibility to provide a sms-deleted event for multiple apps to be aware of the delete operations in MobileMessageDB.
blocking-b2g: --- → 2.0?
Can QA find out if this is a regression?
Keywords: qawanted
Unneeded, this is bug 1007588.
Blocks: 1007588
Keywords: qawanted
blocking-b2g: 2.0? → 2.0+
QA Contact: lolimartinezcr
Comment 18 says this is a dup of 1007588. Why was this set to blocking without dup-ing it. Bhavana?
Flags: needinfo?(bbajaj)
This is a duplicate so marking
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1007588
This is not a duplicate, we need it to fix threading problem described in comment 0. It is owed to the case of Cost Control app deleting an SMS while SMS app is running. This bug would be to fix gaia part of SMS app, and gecko part would be implemented in bug 1023695, creating an event to be handled by SMS app.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Changing the component to SMS application
Component: Gaia::Cost Control → Gaia::SMS
bug 1007588 was the regressing bug, not a duplicate; I answered the previous request from Jason and marked it accordingly in the "blocks" line. Sorry for the confusion.

The solution should be quite easy once we have the Gecko event.
(In reply to Andreas Gal :gal from comment #19)
> Comment 18 says this is a dup of 1007588. Why was this set to blocking
> without dup-ing it. Bhavana?

It was indeed discussed that 1007588 was the blocker bug and not a DUP, which Julien clarifies per comment #23.
Flags: needinfo?(bbajaj)
ETA of the Gecko dependency (bug 1023695) will be 7/11. 
It's under review so please wait a few days.
Some of the work done in Bug 1024835 will be used here.
Hi Marina,

Could you please let us know what SMS exactly should be received so that Cost control\Usage app can recognize it correctly?

Also any other pointers on how to correctly configure SIM card to work with Cost control\Usage app would be appreciated :)

Thanks!
Flags: needinfo?(mri)
Assignee: nobody → azasypkin
Status: REOPENED → ASSIGNED
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S6 (18july)
Flags: needinfo?(mri)
Hi Vicamo and Marina,

Once we fix bug described in the comment 0 using "navigator.mozMobileMessage.ondeleted" we'll still have the following issue: 

If cost control incoming SMS (eg. with balance info) is received when SMS inbox is opened, user will notice that new thread appears and then quickly disappears (when cost control app deletes it). Probably it's not very frequent case, but still it doesn't feel like good UX. It depends on how long it can take for Cost Control app to process and delete that SMS + how long it can take for SMS app to create and then delete new thread.

When we have messages datastore probably other apps that are going to use it will also suffer from such thing. Also what will happen if Cost Control\SMS app is crashed\killed in the middle of it. 

So, we probably can think of different workarounds on SMS side, but I'd like to know whether Gecko\System can take care of it in a cleaner and more scalable way (ideally SMS app shouldn't know about such service SMS at all).

What do you guys think?

Thanks!
Flags: needinfo?(vyang)
Flags: needinfo?(mri)
Hi,

We have silent SMS for payment, but that's not available for apps other than System app. They are delivered to neither SMS database nor mozMobileMessage::onreceived event target but the payment method itself. If we agree to ask System app to send that SMS for you with the silent flag set, I think that would fit the need perfectly. However, this follows some complicated communication between Cost Control and System app.
Flags: needinfo?(vyang)
Hey Vicamo,

here is the requirements from the Cost Control side, as I understood it:

* needs to send a message silently
* needs to receive a message silently
* that message can come from different numbers (not necessarily the same one that the initial destination)
* the said numbers can also send messages that we don't want to silence
* CostControl parses the input message and then delete it and disables the silencing only if it was a valid answer

That's why the current solution uses a quite complex solution involving IAC. I don't really like it so much, but it sounded like the simplest solution to fulfill these requirements. I don't know if the current Silent SMS feature works for this?
Flags: needinfo?(vyang)
Hi, 
the process to update the balance depends on the minimun_delay variable of the config. Currently all the countries have 3 hours as value.

When the user interacts with the widget or directly with the App, the app checks when the last balance request was made, if the mínimun_delay passed then the app proceed to send the message.

For this reason, IMHO the usual case is that the user is not working with sms when these messages are been processed, although no way to ensure this.

Regards
Flags: needinfo?(mri)
(In reply to Julien Wajsberg [:julienw] (PTO Monday 14th July) from comment #30)
> Hey Vicamo,
> 
> here is the requirements from the Cost Control side, as I understood it:
> 
> * needs to send a message silently
> * needs to receive a message silently
> * that message can come from different numbers (not necessarily the same one
> that the initial destination)
> * the said numbers can also send messages that we don't want to silence

There are three related APIs for silent SMS.  {add,remove}SilentNumber to add/remove interested silent SMS.  Received messages with its sender attribute set to one of the registered silent message numbers are only broadcasted inside Gecko chrome process via "sms-received" observer event. For outgoing messages, use that aSilent argument of nsISmsService::Send(). [1]  I think this meets all above conditions unless we have an additional one:

  * messages from a certain address may be silent or non-silent in a certain period of time. Totally unpredictable.

> * CostControl parses the input message and then delete it and disables the
> silencing only if it was a valid answer

As said, a silent message will never be stored into database. So even that's an unexpected answer, the message just can't be put back to database at present.

> That's why the current solution uses a quite complex solution involving IAC.
> I don't really like it so much, but it sounded like the simplest solution to
> fulfill these requirements. I don't know if the current Silent SMS feature
> works for this?

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsISmsService.idl#24
Flags: needinfo?(vyang)
Ok thanks Vicamo.

For v2.0, unless we have partner blockers, I think it's safer and good enough to keep the current way. For later versions we can revisit if we have requests to do it from UX. I personally think it's actually not that bad to see the threads appearing/disappearing for more transparency to the user.
Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

Hey Julien,

Here is a small patch (v2.0 only) that fixes issue in comment 0, other refactoring and improvements will be in master patch only.

Thanks
Attachment #8456311 - Flags: review?(felash)
Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

I think we should land this patch in both master and v2.0 and file a separate bug for the more invasive changes we want to do in master.
Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

I keep my r for another round for unit tests only. The code is fine.

Thanks !
Attachment #8456311 - Flags: review?(felash)
Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

(In reply to Julien Wajsberg [:julienw] from comment #38)
> Comment on attachment 8456311 [details] [review]
> GitHub pull request URL (v2.0)
> 
> I keep my r for another round for unit tests only. The code is fine.
> 
> Thanks !
Thanks for review! Updated unit test with your suggestion.

> I think we should land this patch in both master and v2.0 and file a
> separate bug for the more invasive changes we want to do in master.
Yep, moreover I'll probably have more than one follow-up bug for master...
Attachment #8456311 - Flags: review?(felash)
Just for the record, here are steps that allow to simulate "SIM card with billing support" provided by Marina:

1. Make "getConfigFilePath" function to always return "'js/config/vivo/config.js'" in [1];
2. In [2] add your mobile number to the array of "senders" like "['4850', '+123456789']";
3. (Optional) In [3] set the same mobile number as "destination" like "+123456789";
3. Open Cost Control (Usage) app and configure SIM as "Prepaid";
4. At the balance screen tap on "Update" (now Cost Control sends a message to the number defined in the [3], this message is removed by Cost Control once it's sent);
5. From the number defined in [2] send the following message "2014071108523301110010215.67;15/06/2014" where "15.67" part is actual balance (once it's parsed by Cost Control app and determined as valid it will be automatically removed).

[1] https://github.com/mozilla-b2g/gaia/blob/1246714f168d23d624244e352f8ba5d151a6da9a/apps/costcontrol/js/config/config_manager.js#L104
[2] https://github.com/mozilla-b2g/gaia/blob/1246714f168d23d624244e352f8ba5d151a6da9a/apps/costcontrol/js/config/vivo/config.js#L108
[3] https://github.com/mozilla-b2g/gaia/blob/1246714f168d23d624244e352f8ba5d151a6da9a/apps/costcontrol/js/config/vivo/config.js#L104
Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

r=me

Use insertMockMarkup to insert the mock DOM if you think it's better, otherwise you can leave it like this.

I haven't tried to exercise the whole code with CostControl but I did try to delete threads and at least this doesn't regress :) I trust that you tested the case with CostControl ;)
Attachment #8456311 - Flags: review?(felash) → review+
Comment on attachment 8456686 [details] [review]
GitHub pull request URL (master)

as discussed, this bigger patch won't land here.
Attachment #8456686 - Attachment is obsolete: true
Master: https://github.com/mozilla-b2g/gaia/commit/6f335ef8865e597d83d47621261e4ba844d8bbdf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Tested and working
Hamachi
2.0
Gecko-15afb7d
Gaia-c7b190a

Hamachi
2.1
Gecko-c431020
Gaia-1c9eb3d
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.