959696 - [Messages][Drafts][Refactoring] Move draft saving on visibility change to ThreadUI, and only store drafts if new one is saved

Status: RESOLVED FIXED

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

Description

[Evelyn Eastmond [:evhan55]]

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/message_manager.js#L103-L119

Currently:

MessageManager handles visibility change events for the app, and saves a draft if the app is hidden

Refactor:

ThreadUI should handle this visibility change event as well, and handle the draft saving.  Also, the Drafts.store() call should only happen when the draft is saved, so it should be moved inside the inner if statement.

Comment 1

[Evelyn Eastmond [:evhan55]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15738

- `message_manager.js`
    - remove the visibility change draft-saving behavior
- `thread_ui.js`
    - add a visibility change handler
    - add draft-saving behavior to visibility change handler
- Tests
    - `message_manager_test.js`
        - remove visibility change tests
    - `thread_ui_test.js`
        - add visibility change tests to check that draft saves properly on app hiding
    - manual tests
        - start a new draft => hide app => show app => alter draft => click back => check that it asks to 'replace' draft and not 'save' a new draft

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8366123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15738

works fine but I think we should also move MessageManager.draft to ThreadUI.draft in this bug.

Comment 3

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Comment on attachment 8366123 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/15738
> 
> works fine but I think we should also move MessageManager.draft to
> ThreadUI.draft in this bug.

Also I'd like to question the very existence of MessageManager.draft, but that is maybe for another time.

Comment 4

[Evelyn Eastmond [:evhan55]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #2)
> > Comment on attachment 8366123 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/15738
> > 
> > works fine but I think we should also move MessageManager.draft to
> > ThreadUI.draft in this bug.
> 
> Also I'd like to question the very existence of MessageManager.draft, but
> that is maybe for another time.

Yeah!  Wish I could be around for that chat.

Comment 5

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #2)
> > Comment on attachment 8366123 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/15738
> > 
> > works fine but I think we should also move MessageManager.draft to
> > ThreadUI.draft in this bug.
> 
> Also I'd like to question the very existence of MessageManager.draft, but
> that is maybe for another time.

When you click on a threadless draft and arrive at the new message view, there is no way to know which draft to load in. I could've used the id as a parameter in the hash, like `#new&draft=1`, but that would've required changing a lot of extant code that expects `hash === '#new'` and I was under the impression that hashes were going away eventually. MessageManager.draft exists as a temporary holding station to solve this problem.

Comment 6

[Rick Waldron [:rwaldron]]

Another thought: this only exists because we support multiple threadless drafts.

Comment 7

[Julien Wajsberg [:julienw]]

(In reply to Rick Waldron [:rwaldron] from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #3)
> > (In reply to Julien Wajsberg [:julienw] from comment #2)
> > > Comment on attachment 8366123 [details] [review]
> > > https://github.com/mozilla-b2g/gaia/pull/15738
> > > 
> > > works fine but I think we should also move MessageManager.draft to
> > > ThreadUI.draft in this bug.
> > 
> > Also I'd like to question the very existence of MessageManager.draft, but
> > that is maybe for another time.
> 
> When you click on a threadless draft and arrive at the new message view,
> there is no way to know which draft to load in. I could've used the id as a
> parameter in the hash, like `#new&draft=1`, but that would've required
> changing a lot of extant code that expects `hash === '#new'` and I was under
> the impression that hashes were going away eventually. MessageManager.draft
> exists as a temporary holding station to solve this problem.

Oki, so after bug 881469 we can try to remove it. Thanks for the insight!

Comment 8

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #7)

> Oki, so after bug 881469 we can try to remove it. Thanks for the insight!

Yes, absolutely.

Comment 9

[Rick Waldron [:rwaldron]]

Comment on attachment 8366123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15738

Good to go, r=me

Comment 10

[Evelyn Eastmond [:evhan55]]

Landed and closing: https://github.com/mozilla-b2g/gaia/commit/6c9fac83b743711d02c4a32e45cd145d8a85f7eb

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This was reverted:
https://github.com/mozilla-b2g/gaia/commit/75122a82fb557f6d349a09ef14c37c9e1dddf0dc

Presumably for the same test failures we were seeing on TBPL:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33893843&tree=B2g-Inbound

Comment 12

[Rick Waldron [:rwaldron]]

3 green travis runs: 

https://travis-ci.org/mozilla-b2g/gaia/builds/17997162
https://travis-ci.org/mozilla-b2g/gaia/builds/17997144
https://travis-ci.org/mozilla-b2g/gaia/builds/17997619


Landed https://github.com/mozilla-b2g/gaia/commit/6e232d655b5ad98dddeb4f1c42e206019a1b7137