1207081 - [Messages] Make filtering better for markable thread in InboxView.markReadUnread method

Status: RESOLVED FIXED

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

Description

[kumar rishav (:rishav_)]

Make filtering better for markable thread in InboxView.markReadUnread method

Add this condition (&& !(thread.unreadCount ^ isRead)) to the [1] for better filtering of markable thread.
Modify the unit test accordingly, if required.

[1]
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/inbox/js/inbox.js#L401

Comment 1

[kumar rishav (:rishav_)]

Currently in InboxView.markReadUnread method  (https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/inbox/js/inbox.js#397)
To filter the markable thread, we have following condition:
var markable = thread && !thread.isDraft &&
        (isRead || !thread.getDraft());

Filtering can be better if we add this '&& !(thread.unreadCount ^ isRead) ' to it.
To explain this condition: if thread has unread message and isRead==false (i.e to make it unread), this thread will be skipped and will not considered as markable as thread has already unread messsage.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] s142857:master > mozilla-b2g:master

Comment 3

[mendasanketh]

https://github.com/mozilla-b2g/gaia/pull/32048

Comment 4

[Julien Wajsberg [:julienw]]

Hey Kumar, can you help this contributor properly polishing his patch ?

Comment 5

[kumar rishav (:rishav_)]

Yeah Sure, Julien. Infact i was waiting for him to comment here so that i can proceed with guiding.

Comment 6

[mendasanketh]

sorry

Comment 7

[mendasanketh]

what do you want me to do

Comment 8

[kumar rishav (:rishav_)]

Hi Mendasanketh,
Assuming this is you first Gaia bug and aware of HTML5,javascript and Bugzilla.

First of all you need to build the gaia in your system, to work on the patch/code.
Please follow this:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md

If you feel any problem, feel free to comment or ping on irc channel-> #fxos, #gaia-messaging

Regarding the patch you submitted,
Whenever you submit a patch, raise feedback? (or atleast ni?) flag. and comment if you have any doubt.
I saw you patch, that condition should be added in var markable = thread && !thread.isDraft && (isRead || !thread.getDraft()) && !(thread.unreadCount ^ isRead)), but what you did is wrong.

Comment 9

[kumar rishav (:rishav_)]

if you feel problem in javascript, you read it here : https://developer.mozilla.org/en-US/docs/Web/JavaScript

Nice tutorial it is :)

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] s142857:Bug-1207081 > mozilla-b2g:master

Comment 11

[mendasanketh]

https://github.com/mozilla-b2g/gaia/pull/32055

Comment 12

[kumar rishav (:rishav_)]

Seems like you just copy and pasted the code from above comment :P (one closing bracket is extra, in above code by mistake, sorry for it)

But i need you to understand the code/syntax before submitting the patch.

Did you build the Gaia locally in you system? Did you run you code there? 

Without above step, you cannot move forward. you should have local build of gaia to test your code.

Comment 13

[mendasanketh]

now i did it locally. added it and tested it also

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] s142857:master > mozilla-b2g:master

Comment 15

[mendasanketh]

could you check it out.']
https://github.com/mozilla-b2g/gaia/pull/32059

Comment 16

[kumar rishav (:rishav_)]

I had a look on your patch. It seems you combined your patch with other commits.[i guess you messed up your master now :P so, clean it , create new branch then start your work on it]

Follow these for better workflow:
1. Never work on your master branch.
2. create a new branch (say, Bug-1207081) then work on it.
3. Also, you don't have to create new pull request every time you do some change. Instead you can work on same branch/pull request. At end we can squash all the commits.
4. Once your are donw with patch, run 'make lint' to lint your patch. width (length of code in a line) shouldn't increase more than 80.you can make it to next line. something like this.

 var markable = thread && !thread.isDraft &&
   (isRead || !thread.getDraft()) && !(thread.unreadCount ^ isRead);

P.S - Be careful with identation.

5. After this you should modify the existing unit text also.
you can find related unit test here :-
Suite for markReadUnread: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/inbox/test/unit/inbox_test.js#L607

you should spy on  MessageManager.markThreadRead method (see markReadUnread method) to check the argument passed. (to test if expected argument[thread.id] passed or not)

you can read more about the unit test here: http://sinonjs.org/

Comment 17

[mendasanketh]

how should i modify the test

Comment 18

[kumar rishav (:rishav_)]

Have you worked with unit test before. If not, then i will suggest you to read about this first. You can check the link (http://sinonjs.org/) and see existing code(say for markReadUnread method).

basically you have to set an spy on MessageManager.markThreadRead in the setup, then in test check if that function is called with expected argument or not.

Update your patch and clean it. 
I wil comment there for more understanding. But i strongly recommend to read about unit test and understand it.

Comment 19

[kumar rishav (:rishav_)]

In setup, set spy : something like this:
this.sinon.spy(MessageManager, 'markThreadRead');

then in test check like this
sinon.assert.calledWith(MessageManager.markThreadRead, id, true or false whatever it is); [see existing code]

Hope you read the instructions here: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/README.md
To run the unit test,
Navigate to gaia folder and:
bin/gaia-test

Comment 20

[mendasanketh]

I don’t have any experience with unit tests, but ill try to learn.
tests passed. check if what i did is right

Comment 21

[kumar rishav (:rishav_)]

I have added the comment on patch. patch is fine (except unit test part).
I expect you to learn or go through the unit test first. [like what spy do, what test do, what should i test. Best way is to go through docs first to get some idea. then play around with existing code of sms unit test, to learn it. Becz sinonjs docs won't be enough to understand. Existing code are very helpful. That;s how i learnt :)]
Test passed because you didn't add any test. you just added the spy. see my above comment.

P.S - Please don't copy paste the code. First learn it, then if you want , you can :)

Comment 22

[mendasanketh]

could you reassign it, cuz I lack the technical knowledge to do it.

Comment 23

[kumar rishav (:rishav_)]

didn't get you. what to reassign?

Comment 24

[kumar rishav (:rishav_)]

I am available on irc (#fxos, #gaia-messaging), you can ping me there too :)  [rishav_]

Comment 25

[kumar rishav (:rishav_)]

Hey,
How you doing on the bug? any issue?

Comment 26

[kumar rishav (:rishav_)]

hey, any update ?

Comment 27

[kumar rishav (:rishav_)]

comment if you need help :)

Comment 28

[kumar rishav (:rishav_)]

No update from contributor, Assigning to nobody for now.

Hey Julienw,

I am thinking to fix this in https://bugzilla.mozilla.org/show_bug.cgi?id=1215655

As both codes are related (i.e same method) and need to change a line only.

Thanks

Comment 29

[kumar rishav (:rishav_)]

Fixed in Bug 1215655.
See the bug for patch.

Comment 30

[kumar rishav (:rishav_)]

Hey Kalpesh, if you want, you can give a shot to this bug.
My patch(Bug 1215655) will take more time (waiting for new UX and VD), so better to fix here only. 

Small bug only, need to change only one line, it will be nice if you understand the code, what that code is doing, then make changes. Add unit test too.

Thanks 

Clearing ni? from previous contributor, as no update from him.

Comment 31

[kumar rishav (:rishav_)]

ping  on irc or comment here if you need help :)

Comment 32

[Kalpesh Krishna [:martianwars]]

Hey Kumar,
I'll work on this one. :) Lots of comments here, which ones would help? Should I read the contributor's patch, or should I start from your first comment? Thanks!

Comment 33

[kumar rishav (:rishav_)]

Go with description and comment 1 .

Thanks

Comment 34

[kumar rishav (:rishav_)]

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/inbox/js/inbox.js#L407

Comment 35

[Kalpesh Krishna [:martianwars]]

Kumar,
I don't really understand what's going on. We want the following to be TRUE to make the thread markable :-
#1 thread --> I guess this means that thread variable is not NULL
#2 !thread.isDraft --> thread is not in drafts folder
#3 isRead || !thread.getDraft() --> the thread is either read or the draft is NULL. But didn't we check for draft in condition #2?

Also, What exactly are we checking using the XOR ?
!(thread.unreadCount ^ isRead) --> Either thread should be unread and unreadCount should be 0 or thread should be read and unreadCount is 1. Why is this needed? I didn't really understand. What is unreadCount doing?

Comment 36

[kumar rishav (:rishav_)]

(In reply to Kalpesh Krishna [:martianwars] from comment #35)
> Kumar,
> I don't really understand what's going on. We want the following to be TRUE
> to make the thread markable :-
> #1 thread --> I guess this means that thread variable is not NULL
> #2 !thread.isDraft --> thread is not in drafts folder
i.e thread is a draft (only draft), no any other msg.
> #3 isRead || !thread.getDraft() --> the thread is either read or the draft
thread has draft, i.e it contains other msg other than draft.
> is NULL. But didn't we check for draft in condition #2?
> 
> Also, What exactly are we checking using the XOR ?
See, we selected few threads (lets say, one unread thread and another read thread, then final icon will be mark as read, so here we can skip the thread which is already read, and mark as read only unread thread )
> !(thread.unreadCount ^ isRead) --> Either thread should be unread and
> unreadCount should be 0 or thread should be read and unreadCount is 1. Why
> is this needed? I didn't really understand. What is unreadCount doing?


Hope you got me. If not, you can ask again, i will clarify :)

Comment 37

[Kalpesh Krishna [:martianwars]]

Yeah makes some sense now. So unreadCount is the number of unread messages?

Comment 38

[GitHub Autolander Bot]

Attachment: [gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

Comment 39

[kumar rishav (:rishav_)]

Yes, unreadCount contains no. of unread msg. So, if we want to make a thread unread, making unreadCount = 1, is enough to show thread as unread.
!(thread.unreadCount ^ isRead) => !(have unread msg(T) ^ mark as unread(i.e F)) => !(T) => F => skip the thread.

or 

!(thread.unreadCount ^ isRead) => !(NO unread msg(F) ^ mark as read(i.e T)) => !(T) => F => skip the thread.

Hope it's clear now.

Comment 40

[kumar rishav (:rishav_)]

Are you unable to run unit test locally? 
Few suggestion, hope it help.
1. First and most important, good internet connection :D 
2. Clean the profile and make again, i.e (make really-clean), make DEBUG=1 DESKTOP=0  (assuming you have updated your gaia master)
3.Have latest node, node v4.X and npm 2.X

in final patch,do r=julienw (patch should go through peer or owner of app :) )
 
For test, here you need to assertion in existing test.
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/inbox/test/unit/inbox_test.js#L605

To check if this method is called with expected thread id.
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/views/inbox/js/inbox.js#L414

Thanks

Comment 41

[Kalpesh Krishna [:martianwars]]

I've added a query at Github https://github.com/mozilla-b2g/gaia/pull/33500

Comment 42

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8695329 [details] [review]
[gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

Hope it is okay :)

Comment 43

[kumar rishav (:rishav_)]

Comment on attachment 8695329 [details] [review]
[gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

Looks good to me. Few nits only. Left some comments on github. Fix those (squash commits into one) and once test passed set review flag to me.

Comment 44

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8695329 [details] [review]
[gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

The nit you mentioned doesn't work :( Probably true/false refers to something else. neverCalledWith() works fine :)

Comment 45

[kumar rishav (:rishav_)]

Comment on attachment 8695329 [details] [review]
[gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

one more nit :P

thanks for your work :)

Comment 46

[Steve Chung [:steveck]]

Hi Kalpesh & kumar,
I left a comment on github, please let me know about your thoughts, thanks!

Comment 47

[kumar rishav (:rishav_)]

Hi Steve,
Agree with you. Left some comments on github.

Hi Kalpesh,
Have a look on steve's suggestion and modify the patch accordingly.


Thanks

Comment 48

[Steve Chung [:steveck]]

Comment on attachment 8695329 [details] [review]
[gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

Hey Kalpesh, please set the review again once you're ready.

Comment 49

[Kalpesh Krishna [:martianwars]]

Comment on attachment 8695329 [details] [review]
[gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

Steve, Rishav,
I've fixed the PR and I hope it is correct now :)

Comment 50

[Steve Chung [:steveck]]

Look good! Please address the nit from Kumar and squash into 1 commit, thanks.

Comment 51

[Kalpesh Krishna [:martianwars]]

@Steve,
Squashed and pushed!

Comment 52

[kumar rishav (:rishav_)]

Seems fine now :)
gaia try green,
adding checkin-needed

Thanks kalpesh for ur work :)

Comment 53

[Kalpesh Krishna [:martianwars]]

Rishav,
I've added a need info for https://bugzilla.mozilla.org/show_bug.cgi?id=1183074 . Do help me with that bug. Also, while I continue with the docs, could you tell me about some other bug I could get started with?

Comment 54

[Steve Chung [:steveck]]

Comment on attachment 8695329 [details] [review]
[gaia] martiansideofthemoon:my-code-fix3 > mozilla-b2g:master

Thanks for the contribution \o/

Comment 55

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/c4aff7918fecba968a1e613f418bdf5a37eef528

Comment 56

[kumar rishav (:rishav_)]

Hey Kalpesh,
Thanks for your contribution. Check your docs bug, i had listed few bugs there.
I will clear other ni? soon :)
Thanks