1010216 - [Messages][Drafts] Improve consistency of the draft code in ThreadUI and ThreadListUI

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Once bug 881469 lands, we'll be able to:

* remove the use of ThreadUI.draft
* possibly use ThreadUI.handleDraft in ThreadUI.enterThread
* possibly get rid of ThreadListUI.draftLinks
* possibly use a "draftId" dataset instead of abusing "threadId" for drafts.

Comment 1

[Julien Wajsberg [:julienw]]

(maybe in another bug) possibly make thread-bound and thread-less drafts behave more alike.

Comment 2

[Oleg Zasypkin [:azasypkin]]

We've just noticed that there is an inconsistency in how "ThreadListUI.draftLinks" map is handled. It's supposed that we store <a> references in this map [1], but "removeThread" method expects <li> reference here instead [2].

So in the scope of this bug we should revisit/fix this too.

[1] https://github.com/mozilla-b2g/gaia/blob/09ffd07964de1ad15d7ab34ab89c78bdf223605a/apps/sms/js/thread_list_ui.js#L654

[2] https://github.com/mozilla-b2g/gaia/blob/09ffd07964de1ad15d7ab34ab89c78bdf223605a/apps/sms/js/thread_list_ui.js#L294

Comment 3

[Oleg Zasypkin [:azasypkin]]

Will take a look if I can improve something here without major rewrite. My main goal currently is to get rid of direct InboxView->ConversationView dependency via ConversationView.draft to unblock bug 1155509.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

Comment 5

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

Hey Julien,

May I have your early feedback on the patch? I've left my doubts and questions at github.

Thanks!

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

So there are good things in this patch. I'm quite sad that we don't change handleDraft but because this is mostly to unblock the split, let's do that and file a separate bug for the rest of the work.

Also in a quite short term (let's hope june) drafts will be stored along with threads directly in the same DB. So let's not spend too much time refactoring and thinking too much now as this would be wasted work anyway.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #6)
> Comment on attachment 8601454 [details] [review]
> [gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master
> 
> So there are good things in this patch. I'm quite sad that we don't change
> handleDraft but because this is mostly to unblock the split, let's do that
> and file a separate bug for the rest of the work.

Yep, generalizing handleDraft deserves separate patch especially in the light of future split of Composer and Conversation.

Hey Steve, since Julien is on PTO could you please review the patch?

Thanks!

Comment 8

[Steve Chung [:steveck]]

The patch looks great! I only some questions on github and nits. BTW the there are some weird failed unit test on treeherder, not sure why it happens.

Comment 9

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #8)
> The patch looks great! I only some questions on github and nits. 

Thanks for review! Replied and nits are fixed in a separate commit.

> BTW the there are some weird failed unit test on treeherder, not sure why it happens.

It looks like Treeherder picks/groups other PRs that contain the same commits as target one - I have PR for bug 1010216 (with failing tests :) ) that is based on the same commits + WIP one.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8601454 [details] [review]
[gaia] azasypkin:bug-1010216-improve-drafts > mozilla-b2g:master

Thanks for the refactoring for drafts, now it looks clear and tidy :)

Comment 11

[Oleg Zasypkin [:azasypkin]]

Thanks!

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/95518668b6f41f000d674b7e98bdc97586886a6c