1176976 - [Messages][Drafts] Remove the draft saving/replacing action menu

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

After Bug 1144132 landed, the activity behavior will change and affect the daft behavior in message app. We might need to remove the action menu and simply save the draft directly to avoid the confusing.

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: sms draft v1.png

Adding spec for draft handling improvements provided by Jenny: short-term (that should be handled in this bug) and long/mid-term (once we have undo support, + it was modified a bit during discussion).

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Comment 3

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Hey Julien,

May I have your early feedback on this? I have left several questions at GitHub to get your opinion.

Thanks!

Comment 4

[Oleg Zasypkin [:azasypkin]]

Writing some tests at the moment to improve drafts integration tests coverage.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

I think this looks good!

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Hey Julien,

Finally I was able to resolve issues with draft integration tests I had, fixed/add required unit tests and updated approach to show "draft saved" banner in Inbox view.

I've made some big changes and did a dozen of rebases since feedback was given, so I've squashed commits into one and added few more clarifying comments. So it should be ready for the first review round :)

Thanks!

Comment 7

[Julien Wajsberg [:julienw]]

I started looking at the patch but won't finish today -- I started too late, sorry :(

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

sorry, looks like this needs a full rebase :(

Comment 10

[Oleg Zasypkin [:azasypkin]]

Hey Morpheus,

In this bug we're trying to remove all draft related dialogs (save/replace/delete draft) and save/delete draft automatically whenever it makes sense, but there is one case I'm not sure about:

When user enters conversation with saved draft, selects all messages and removes them. Currently on master user is asked what to do with the draft - save or delete (though I see it's somewhat broken in master, oops :)).

So what do you think we should do here? I'd say we still have to automatically save draft in this case as user explicitly didn't ask to remove draft, but we need your expert UX advice on this :) 

Thanks!

Comment 11

[Morpheus Chen [:morpheus] UX]

I agree with Oleg's suggestion. Since user chooses all messages to delete instead of deleting the entire thread, it means user may want to delete all messages but keep the thread. We need to automatically save the draft for any possibility user wants to edit later. 

However, I am wondering if we can provide the draft as an item on the bottom for user to select in "Delete message" view. So, when all selected, user explicitly wants to delete the thread. When all messages selected except draft, it means user wants to keep the draft. Does that make sense and is it feasible?  

If it's feasible, I can provide detailed spec for this feature. If not, we can just automatically save the draft like what Oleg suggests.

Let me know if any questions,
thanks.

Comment 12

[Oleg Zasypkin [:azasypkin]]

Hey Morpheus,

(In reply to mochen from comment #11)
> I agree with Oleg's suggestion. Since user chooses all messages to delete
> instead of deleting the entire thread, it means user may want to delete all
> messages but keep the thread. We need to automatically save the draft for
> any possibility user wants to edit later. 
> 
> However, I am wondering if we can provide the draft as an item on the bottom
> for user to select in "Delete message" view. So, when all selected, user
> explicitly wants to delete the thread. When all messages selected except
> draft, it means user wants to keep the draft. Does that make sense and is it
> feasible? 

> If it's feasible, I can provide detailed spec for this feature. If not, we
> can just automatically save the draft like what Oleg suggests.

Yeah, I think it's feasible, though it requires quite a lot of work right now as we don't support heterogeneous items in our selection code. So let's go with auto-saving in this bug and return to this new concept once we have free cycles. Who knows how we'll look like once NGA is done :)

Thanks!
 
> Let me know if any questions,
> thanks.

Comment 13

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Hey Julien,

It should be ready for the first review round.

One old issue will become more visible after this patch (see bug 1080504), namely first app instance Inbox will display draft state which it had when it was initially created by activity (second app instance), following updates won't be reflected in the first app instance until app is restarted. Previously we had this issue only if second instance is moved to background, but now we always create draft to back up activity and I've removed draft removal from ConversationView.handleDraft.

I was thinking to fix it separately in bug 1080504, but tell if you want me to fix the issue completely in this PR.

Thanks!

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1176976-remove-draft-dialogs > mozilla-b2g:master

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 8656211 [details] [review]
[gaia] julienw:1176976-remove-draft-dialogs > mozilla-b2g:master

Just a rebased version so that I can have a look at the try result.

Comment 16

[Julien Wajsberg [:julienw]]

Sorry, I started reviewing again but I couldn't finish. I added comments on the rebased pull request by mistake, sorry... The rebased version has a failing test as well.

The conflicts were in the unit tests only; take care that you don't remove some tests that were added in bug 1199593.

I'll finish the review when I come back next week !

Comment 17

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (PTO Sept 2nd -> Sept 10th) from comment #16)
> Sorry, I started reviewing again but I couldn't finish. I added comments on
> the rebased pull request by mistake, sorry... 

I've replied on the rebased PR's comment :)

> The rebased version has a failing test as well.
> 
> The conflicts were in the unit tests only;

Just rebased original PR, fixed conflicts.

> take care that you don't remove some tests that were added in bug 1199593.

I believe I've removed some during rebasing, but I think there were related to "discard" draft/not-sent-message cases that we don't need anymore - please tell me if you think otherwise :)

Thanks!

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

pffew,
I finished my review !

Please put all changes in a separate commit for an easy 2nd review pass :)

Comment 20

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Should be ready for the second round of review! Hope I haven't missed anything :)

Thanks!

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

I left few comments in the additional commit: https://github.com/azasypkin/gaia/commit/b1b0dd4e8eaa410be860d2caf27bf300331a6642

it's r=me with the changes, but I'm fine if you want a 3rd review with a 3rd commit :)

Thanks for this huge work, I's really satisfying to see this landing.

Comment 22

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Added 3rd commit, could you please take a look?

Thanks!

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

ship it !

(with a green try :) )

Comment 24

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #23)
> Comment on attachment 8631656 [details] [review]
> [gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master
> 
> ship it !
> 
> (with a green try :) )

Thanks a lot for comprehensive review!

Treeherder is green, so landed.

Master: https://github.com/mozilla-b2g/gaia/commit/08785512e40df469a3f1bd6a77b723f230b389a4

Comment 26

[Julien Wajsberg [:julienw]]

\o/