Closed
Bug 942076
Opened 12 years ago
Closed 10 years ago
[WAP push] When DUT receives SI message with delete action attribute as a wap push with same SI-ID is opened, opened one will be closed.
Categories
(Firefox OS Graveyard :: Gaia::Wappush, defect)
Tracking
(b2g-v1.3T affected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
b2g-v1.3T | --- | affected |
People
(Reporter: echu, Assigned: gsvelto)
Details
(Whiteboard: permafail)
Attachments
(2 files)
An opened SI message will not be closed when a new SI message with same ID and delete action attribute arrives.
* Build Number
Gaia: ce276842c9ac1746073271fb736dfdb626a89240
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/36c4c667b9f2
BuildID 20131121004002
Version 26.0
* Reproduce Steps
1. Send 2 SI messages with same SI-ID.
2. Open one of the SI message and do not close it.
2. Send another SI message with same SI-ID and delete action attribute as step 1.
* Expected Result
Opened SI message should be closed.
* Actual Result
Opened SI message is not closed.
* Occurrence rate
100%
Assignee | ||
Updated•12 years ago
|
Component: Gaia::System → Gaia::Wappush
Hardware: x86_64 → ARM
Updated•12 years ago
|
Whiteboard: burirun1.3-2
Updated•11 years ago
|
status-b2g-v1.3T:
--- → affected
Comment 1•10 years ago
|
||
NI on Gabriele to take a look, this is a long standing issue and doesn't seem to be tracked by flags or by block/dependencies. If this is being actively tracked, sorry for the noise.
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 2•10 years ago
|
||
This is a valid bug and I have the infrastructure in place to fix it now; leaving the NI so I'll do it ASAP.
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8634086 [details] [review]
[gaia] gabrielesvelto:bug-942076-close-si-id-when-deleted > mozilla-b2g:master
Big patch. This fixes a lot more than just this bug: once I moved updating the UI via an event-based system I would have introduced some regressions if I had left the existing code as-is so I preferred fixing everything in one go. Here's a summary of the changes:
- The database code now sends 'new', 'update', 'discard' and 'delete' events whenever a message is added to the database, updated, discarded or deleted. This events are used to drive the UI.
- The database does not delete SI/SL messages right away, instead it marks them as already viewed by adding them to a separate table and then deletes them the next time the app is started. This is explicitly made to ensure reliable deletions (which we cannot do straight away due to LMK) while preserving messages for updates, etc...
- The UI is updated in response to the events emitted by the database. The changes required for this address a number of issues:
- If the current displayed SI message is deleted by an incoming SI message with action=delete set the app is closed
- Notifications for deleted messages are now removed
- If the current displayed SI message is updated by a newer incoming SI message then the UI is updated
- If the user would tap on a message notification of a different type while the app was open the UI would display both the old and the new messages. Now message panels are properly hidden when switching message types with app open.
- If a message had an expiration date which was later the moment in which it was received we would save the message. However if the user would open such message after the expiration date we'd show it as still valid. We know show all expired messages correctly.
- Last but not least errors are now printed out correctly to the log
- Unit-tests were tweaked to accommodate for the changes and more were introduced to cover the new functionality. Those in wappush_test.js are still more like poor man's integration tests rather than proper unit-tests which is something we should address but too large of a change for this bug.
Attachment #8634086 -
Flags: review?(felash)
Comment 5•10 years ago
|
||
Comment on attachment 8634086 [details] [review]
[gaia] gabrielesvelto:bug-942076-close-si-id-when-deleted > mozilla-b2g:master
There are a lot of good things in this patch.
I didn't try it for real (I don't really know how to do it BTW) but I left some comments. Tell me what you think !
Attachment #8634086 -
Flags: review?(felash)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8634086 [details] [review]
[gaia] gabrielesvelto:bug-942076-close-si-id-when-deleted > mozilla-b2g:master
Thanks for the review Julien, testing can be done via the emulator using the sms pdu command but it's quite cumbersome (see bug 891762 comment 11). I did test all the scenarios I possibly could; it would be nice though to add some kind of debugging mode to the app that would allow testing by mocking the incoming messages rather than having to use the emulator. Or maybe, you know, integration tests :-P
Anyway I've addressed all your comments, updated the unit-tests to reflect the changes and pushed a separate patch with the changes.
Attachment #8634086 -
Flags: review?(felash)
Comment 7•10 years ago
|
||
Comment on attachment 8634086 [details] [review]
[gaia] gabrielesvelto:bug-942076-close-si-id-when-deleted > mozilla-b2g:master
r=me with the latest nits and a green try
Feel free to request a last review if you feel like it though.
Attachment #8634086 -
Flags: review?(felash) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Thanks for the review, I've adjusted the ParsedMessage.load() functionality and documentation and added unit-tests covering it. I'll wait for the try-run to pass and then merge; the changes are trivial enough that I don't think another review is needed.
Assignee | ||
Comment 9•10 years ago
|
||
Try is green: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=4225b1e35c30f325ed00b3f7aed2d74498b24497 merged to gaia/master d102be422e54eb73f512ee9acf6bf10fa294232d
https://github.com/mozilla-b2g/gaia/commit/d102be422e54eb73f512ee9acf6bf10fa294232d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•