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

Status: RESOLVED FIXED

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

Description

[sasikala]

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

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

Do a checkInputs call after appending messages. Also added tests for this scenario.

Comment 3

[Leo]

Attachment: Pull request for "Select all" button enabled for new SMS in edit screen

This pull request will fix the "Select all" button disabled when new sms receives in both thread and thread_list

Comment 4

[sasikala]

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,

Comment 5

[Borja Salguero [:borjasalguero]]

I've added a small comment. Please take a look! Thanks

Comment 6

[Alex Keybl [:akeybl]]

Please nominate for approval-gaia-v1 once this has r+ and lands, but this is not a blocker.

Comment 7

[Borja Salguero [:borjasalguero]]

Hi sasikala, please take a look to the comments in the PR. Thanks!

Comment 8

[Leo]

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 9

[Fernando Campo (:fcampo)]

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

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

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 11

[Fernando Campo (:fcampo)]

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

Cool! thanks for the super quick fix :)

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

Can you merge it as well? Don't have commit rights.

Comment 13

[Leo]

Attachment: Proposed changes on patch

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.

Comment 14

[Julien Wajsberg [:julienw]]

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...

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

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 :-)

Comment 16

[Leo]

(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.

Comment 17

[Leo]

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.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Thanks for trying the PR, I rebased master upon and fixed the throwing.

Comment 19

[Julien Wajsberg [:julienw]]

(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.

Comment 20

[Julien Wajsberg [:julienw]]

ok, fine, too late.

Comment 21

[Leo]

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,

Comment 22

[Julien Wajsberg [:julienw]]

"leo.bugzilla": please comment now in Bug 859233 :)

Comment 23

[Julien Wajsberg [:julienw]]

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 !

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

: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.

Comment 25

[Alex Keybl [:akeybl]]

(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.

Comment 27

[Julien Wajsberg [:julienw]]

Dupe Bug 859233 was leo+ and this patch fixes Bug 859233, so requesting leo+ here.

Comment 28

[Borja Salguero [:borjasalguero]]

Comment on attachment 737475 [details] [diff] [review]
Proposed changes on patch

This one was fixed here... https://github.com/mozilla-b2g/gaia/commit/d28a7e559cc62841d622b0f74410c1399ed925a8 right?

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted d28a7e559cc62841d622b0f74410c1399ed925a8 to:
v1-train: 7be4c5e1827c7c5789d46ebf63ddb282d4fea156

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 737475 [details] [diff] [review]
Proposed changes on patch

cleaning things up

Comment 31

[Angela Hubenya]

UCID: owd-7458