Closed Bug 857942 Opened 7 years ago Closed 7 years ago

[SMS] "Select all" button in Edit Messages screen is not enabled when we receive a new sms

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
minor

Tracking

(blocking-b2g:leo+, b2g18+ fixed)

RESOLVED FIXED
1.1 QE1 (5may)
blocking-b2g leo+
Tracking Status
b2g18 + fixed

People

(Reporter: sasikala.paruchuri8, Assigned: janjongboom)

References

()

Details

(Whiteboard: [TD-8676])

Attachments

(1 file, 2 obsolete files)

1. Title : "Select all" button in Edit Messages screen is not enabled when we receive a new sms
2. Precondition : SMS should be working and should contain some messages.
3. Tester's Action : Messages - Open any message - Select Edit - click on "Select all" button - receive new sms from same number
4. Detailed Symptom (ENG.): The Select All button is not enabled when we receive a new message.The user should click on received new message for selecting.
5. Expected : The "Select All" button should be enabled when we receive a new message in edit screen automatically.
6.Reproducibility: Y
1)Frequency Rate : 100%
7.Gaia Revision: 8e7262e3d98ea9574d7f79c1e890ad80cfa40c27
Assignee: nobody → janjongboom
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch PatchSplinter Review
Do a checkInputs call after appending messages. Also added tests for this scenario.
Attachment #735064 - Flags: review?(francisco.jordano)
Assignee: janjongboom → nobody
Duplicate of this bug: 849213
blocking-b2g: --- → leo?
This pull request will fix the "Select all" button disabled when new sms receives in both thread and thread_list
Attachment #735163 - Flags: review?(fbsc)
Hi Borja Salguero,

When we are in edit screen,if a new sms is received it is not updating in that edit screen but the sms is existing in the thread_list with the latest gaia code.

We have tested this patch in gaia revision:1cd6866c35520a1c75f991853695ffdbc1d3d887

This issues occurs both in thread and thread_list screen.This will fix will work in both the cases.Please review the patch and provide your comments.

Thanks,
I've added a small comment. Please take a look! Thanks
Please nominate for approval-gaia-v1 once this has r+ and lands, but this is not a blocker.
Assignee: nobody → leo.bugzilla.gaia
blocking-b2g: leo? → -
tracking-b2g18: --- → +
Hi sasikala, please take a look to the comments in the PR. Thanks!
Whiteboard: [TD-8676]
Flags: needinfo?(sasikala.paruchuri8)
Hi Borja,

I have done the changes with the comments given by you.
I will test it once and upload the patch
I will update the new pull request.

Thanks
Comment on attachment 735064 [details] [diff] [review]
Patch

As commented on github, code looks good enough for the r+ but tests are failing, so can't approve it till situation is fixed
Hi Fernando, tests are failing as well on master, so I guess there has been merged in something that shouldn't have.

Anyway the messaging app tests all pass now. (Y)
Comment on attachment 735064 [details] [diff] [review]
Patch

Cool! thanks for the super quick fix :)
Attachment #735064 - Flags: review?(francisco.jordano) → review+
Can you merge it as well? Don't have commit rights.
Attached patch Proposed changes on patch (obsolete) — Splinter Review
Please check and review the attached proposed patch and provide your comments.
In the proposed(735064) delete functionality is not working due to error with "map function".
Please check my proposed changes and add it to your PR before merging.
Attachment #735163 - Attachment is obsolete: true
Attachment #735163 - Flags: review?(fbsc)
Attachment #737475 - Flags: review?(oteo)
Attachment #737475 - Flags: review?(janjongboom)
Attachment #737475 - Flags: review?(fbsc)
Attachment #737475 - Flags: review?(akeybl)
Flags: needinfo?(sasikala.paruchuri8)
Just discussed with Sharaf that his patch (attachment 737475 [details] [diff] [review]) will be done in Bug 859233 instead.

Let's finish the patch for this specific bug now :)

I know it's already r+ but I added a comment in the PR as I'm concerned with the performance impact of this patch...
Julien, this is the same approach as we have in the thread UI. The query is hit on 2 occasions: one time per delete action, and in checkInputs, which will only fire if you are in the actual edit screen. Creating an array to store that information rather than querying won't yield amazing optimizations. Simplicity first here I'd say. Are you OK with that? If so, I'll merge :-)
Target Milestone: --- → Leo QE1 (5may)
(In reply to janjongboom from comment #15)
> Julien, this is the same approach as we have in the thread UI. The query is
> hit on 2 occasions: one time per delete action, and in checkInputs, which
> will only fire if you are in the actual edit screen. Creating an array to
> store that information rather than querying won't yield amazing
> optimizations. Simplicity first here I'd say. Are you OK with that? If so,
> I'll merge :-)

I believe this makes sense to use query rather using array as there is not going to be big degradation of perfomance.
I tried the patch from janjongboom, but the "inputs.map" funtion in delete operation is throwing error. Please check on this.

Also the master code is different from the PR.

Iam changing the assignee name as he has the active PR for merging.
Assignee: leo.bugzilla.gaia → janjongboom
Thanks for trying the PR, I rebased master upon and fixed the throwing.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to janjongboom from comment #15)
> Julien, this is the same approach as we have in the thread UI. The query is
> hit on 2 occasions: one time per delete action, and in checkInputs, which
> will only fire if you are in the actual edit screen. Creating an array to
> store that information rather than querying won't yield amazing
> optimizations. Simplicity first here I'd say. Are you OK with that? If so,
> I'll merge :-)

When you have 1000 threads and 1000 sms in one thread (see the x-heavy workload that recently landed on master), I think this will lead to bad performance.

Since I know we'll do this again at one point, I won't block on this. But I'd like that we don't hide the querySelector behind the getter, and rather that we actually do the call where we need the information. And at this place, please add a comment with a link to this bug so that we don't forget that use case when we refactor this for performance stuff.
ok, fine, too late.
Hi Julien Wajsberg,

As per your comments in comment 14,I can attach this patch in the bug ID:859233.
You asked to rebase the code and merge the patch to the latest.With latest code the basic delete functionality itself not working.
So,even if i do changes with latest code i can't test the code whether this functionality(which i have a patch) is working or not
Can you please suggest me how to handle this delete patch in the latest code?

Thanks,
Flags: needinfo?(felash)
"leo.bugzilla": please comment now in Bug 859233 :)
Flags: needinfo?(felash)
master: d28a7e559cc62841d622b0f74410c1399ed925a8

Jan, for your future bugs, would you please put the commit hash when resolving the bug to fixed ?

Also, should this go to v1-train ?

Thanks !
Flags: needinfo?(janjongboom)
blocking-b2g: - → leo?
:julienw, sorry forgot to do it on this PR :-) I've leo?'ed this so the people in triage can figure out whether this should be in v1-train.
Flags: needinfo?(janjongboom)
(In reply to Jan Jongboom [:janjongboom] from comment #24)
> :julienw, sorry forgot to do it on this PR :-) I've leo?'ed this so the
> people in triage can figure out whether this should be in v1-train.

See https://bugzilla.mozilla.org/show_bug.cgi?id=857942#c6, let's get an approval nomination on the patch with risk/reward but not block.
blocking-b2g: leo? → -
Duplicate of this bug: 859233
Dupe Bug 859233 was leo+ and this patch fixes Bug 859233, so requesting leo+ here.
blocking-b2g: - → leo?
blocking-b2g: leo? → leo+
Uplifted d28a7e559cc62841d622b0f74410c1399ed925a8 to:
v1-train: 7be4c5e1827c7c5789d46ebf63ddb282d4fea156
Comment on attachment 737475 [details] [diff] [review]
Proposed changes on patch

cleaning things up
Attachment #737475 - Attachment is obsolete: true
Attachment #737475 - Flags: review?(oteo)
Attachment #737475 - Flags: review?(janjongboom)
Attachment #737475 - Flags: review?(akeybl)
Flags: in-moztrap?
UCID: owd-7458
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.