1016228 - [Collections App] Rename Collections

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Amir Nissim (:amirn)]

User should be able to change the name of a collection

Comment 1

[Peter Dolanjski [:pdol]]

I think this should be lower priority than the other features.  We may be able to live without it in 2.0 given users tend to use the preconfigured collections.

Comment 2

[Kevin Grandon :kgrandon]

I don't really see how this depends on bug 1016227, so removing it for now. Please re-add it if it's really necessary. Just trying to create a clean dependency tree. Thanks.

Comment 3

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

Thanks guys

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8477368 [details]
Github pull request

I left a note on GH about using the gaia-header WC. I also think it would be good to wait to have Amir take a look at this.

I think writing an integration test would be valuable if we can. I don't mind taking this up after this lands though, so I will file a but for it but we can land without it. Thanks!

Comment 5

[Amir Nissim (:amirn)]

I think we are missing some information about this feature.

Renaming a collection should have the same result as creating a new custom collection.

This means it must be a QueryCollection and the E.me search parameter should be {query: collection.query} where collection.query === collection.name and 'name' is the new value from the rename action.
We also need to issue new web apps and background requests to update collection.webicons and collection.background.

As an example, compare the collection created after applying the patch and renaming 'Music' to 'Musicals' with a custom 'Musicals' collection.
The web results are different because the renamed collection is still using categoryId=142 (Music) as the search options.

So, when renaming a QueryCollection we need to update:
- collection.query
- collection.name
- collection.webicons
- collection.background
- collection.icon

When renaming a CategoryCollection we also need to:
- delete collection.categoryId
- delete collection.cName

A patch is worth a thousand words :) maybe this will do a better job than my explanation above:
https://github.com/EverythingMe/gaia/commit/f6a804ef1ed232a2a16f1d7866fb0b42d3aca9a7

Comment 6

[Cristian Rodriguez (:crdlc)]

Hi all,

   Regarding to comment 5, I would like to have the opinion of UX and product as well. I can do that Amir explained perfectly but I prefer a double check with them before doing more stuffs.

   We have to scenarios with this example:

Chris with his new FFOS changes the "Social" collection to "My friends"

1) Rename a collection and keep all the information doing the query with the original name

Social will appear as "My friends" but the query to show web icons will be done with "Social" (basically just a change of name)

or....

2) Rename a collection and we will do the query with the new name to obtain different results and the new background related to new search

Social will appear as "My friends" and the results show web icons related to "My friends" with a new background

Summarizing, is the 'rename' action just a change of name or a semantic change?

Thanks a lot guys

Comment 7

[Amir Nissim (:amirn)]

The behavior described in Comment 5 is what we implemented in v1.x (bug 911550):
https://github.com/mozilla-b2g/gaia/pull/12553/files#diff-e1bb11dc3cd2b3a2d5a57c50eeed39c2R132

Comment 8

[Cristian Rodriguez (:crdlc)]

Amir, the pull request implements both solutions:

1) Just a change of name -> first commit

https://github.com/crdlc/gaia/commit/508876c6e6f5d47c81eeda4e2c0801f73ac7e24d

2) Semantic change like previous versions -> squash the two commits

https://github.com/crdlc/gaia/commit/dbf4f81ebc30b591712cfc77cdf8defe3c2043ac

Comment 9

[Peter Dolanjski [:pdol]]

(In reply to Cristian Rodriguez (:crdlc) from comment #6)
> Social will appear as "My friends" and the results show web icons related to
> "My friends" with a new background
> 
> Summarizing, is the 'rename' action just a change of name or a semantic
> change?

+Jacqueline for comment.

Comment 10

[Francis Djabri [:djabber]]

Also adding Maria

Comment 11

[Maria Sandberg [:mushi]]

I'll bring this up with the UX team to discuss. 

I'm worried we don't explain to the user well enough how the smart collections work and what the relationship between the collection name and contents (whichever option we choose to go with) is. Without explaining the behavior I think we'll see lots of confusion from users. We also need to consider how this will relate to naming of groups. 

I'll respond back when I've chatted with the team.

Comment 12

[Amir Nissim (:amirn)]

Comment on attachment 8477368 [details]
Github pull request

Clearing r? until we have an answer from UX. Please ask again when relevant.

Comment 13

[Maria Sandberg [:mushi]]

Let's have a rename just be a change of name that has no effect on the content in the collection. 

From our studies we know that users interpret the smart collections as full of preloaded apps. It's also unlikely that users are aware of the connection between the collection name and the query. Having the content change on a name change would be unexpected to the user.

Comment 14

[Amir Nissim (:amirn)]

Maria, should this apply to custom collections as well?
If a user created a custom 'dogs' collections and later renames it to 'cats' - wouldn't it be confusing to get 'dogs' results in a collection named 'cats'?

--

Cristian, it would be great if we can still use the second patch for collection.searchOptions and a modified version of collection.rename.

Added a comment on Github on how to tell that a collection has been renamed:
https://github.com/crdlc/gaia/commit/508876c6e6f5d47c81eeda4e2c0801f73ac7e24d#commitcomment-7602776

Comment 15

[Cristian Rodriguez (:crdlc)]

I prefer to wait for Maria's answer to your question because according to comment 13 the second commit will disappear 

(In reply to Amir Nissim (:amirn) from comment #14)
> Maria, should this apply to custom collections as well?
> If a user created a custom 'dogs' collections and later renames it to 'cats'
> - wouldn't it be confusing to get 'dogs' results in a collection named
> 'cats'?
> 
> --
> 
> Cristian, it would be great if we can still use the second patch for
> collection.searchOptions and a modified version of collection.rename.
> 
> Added a comment on Github on how to tell that a collection has been renamed:
> https://github.com/crdlc/gaia/commit/
> 508876c6e6f5d47c81eeda4e2c0801f73ac7e24d#commitcomment-7602776

Comment 16

[Maria Sandberg [:mushi]]

Yes, the behavior should be consistent for preloaded and custom collections. 

Amir, I agree that in your example it may seem counter intuitive to not have the contents reflect the name of the collection. Most users won't be aware of the potential of a connection however, to them it will be no stranger than renaming a folder on their computer. If they do indeed want a folder with results for 'cats', they can just create a new one with that name.

Comment 17

[Cristian Rodriguez (:crdlc)]

Attachment: Edit collection screen

The same idea than bookmarks

Comment 18

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8477368 [details]
Github pull request

Addressed Maria's suggestion

Comment 19

[Eric Pang [:epang] UX]

Comment on attachment 8484084 [details]
Edit collection screen

screen shot looks good to me, thanks!

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8477368 [details]
Github pull request

I think the code seems fine to me, but would like Amir to leave the final R+ if that's ok with you since he's already commented quite a bit on this.

I guess this also depends on getting the bookmarks patch relanded?

Comment 21

[Jacqueline Savory [:jsavory] UX]

Removing flag as Maria has answered for UX

Comment 22

[Cristian Rodriguez (:crdlc)]

Yes, you're right, it depends on getting the bookmarks patch relanded. I am working on that right now

(In reply to Kevin Grandon :kgrandon from comment #20)
> Comment on attachment 8477368 [details]
> Github pull request
> 
> I think the code seems fine to me, but would like Amir to leave the final R+
> if that's ok with you since he's already commented quite a bit on this.
> 
> I guess this also depends on getting the bookmarks patch relanded?

Comment 23

[Cristian Rodriguez (:crdlc)]

code ready for review and happy tree

Comment 24

[Amir Nissim (:amirn)]

Comment on attachment 8477368 [details]
Github pull request

Looks great. Love the icon in the rename page :)

One thing I noticed is the user can't tap 'done' if the name was not changed. Not sure if it's a bug, but it did confuse me at first.

Comment 25

[Cristian Rodriguez (:crdlc)]

Yes, you are right, the button is disabled while there is no changes by design like editing a contact for instance. Thanks for your time Amir!!

(In reply to Amir Nissim (:amirn) from comment #24)
> Comment on attachment 8477368 [details]
> Github pull request
> 
> Looks great. Love the icon in the rename page :)
> 
> One thing I noticed is the user can't tap 'done' if the name was not
> changed. Not sure if it's a bug, but it did confuse me at first.

Comment 26

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/91edecd83e33b4704e5774e15e5b8b45c0f28d48

Comment 27

[Cristian Rodriguez (:crdlc)]

Hi Peter, do you think that this feature should be in 2.1? If so I will ask for approval, thanks a lot

Comment 28

[Kevin Grandon :kgrandon]

I don't think we could uplift this due to the fact that there's new strings and we're past string freeze =/

Clearing Peter's flag for now, but feel free to re-flag if needed.

Comment 29

[Cristian Rodriguez (:crdlc)]

ok, I thought that we can until 09/14. I was wrong obviously XD

Comment 30

[Kevin Grandon :kgrandon]

(In reply to Cristian Rodriguez (:crdlc) from comment #29)
> ok, I thought that we can until 09/14. I was wrong obviously XD

Actually, I'm sorry - I think we can wait, and I had the wrong dates. It's my fault XD. We can request uplift and see.

Comment 31

[Kevin Grandon :kgrandon]

Comment on attachment 8477368 [details]
Github pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): This is a new feature.
[User impact] if declined: Same as 2.1, users will just not be able to edit collections. This is a feature we previously had in 1.4.
[Testing completed]: Manual and unit testing.
[Risk to taking this patch] (and alternatives if risky): It's fairly well silo'd so should be low risk.
[String changes made]: Yes, new strings to support the feature.

Comment 32

[Kevin Grandon :kgrandon]

I suppose we should flag this as late-l10n because we are requesting uplift, but let's remove it if it's not needed or we don't receive uplift approval.

Comment 33

[Cristian Rodriguez (:crdlc)]

v2.1: https://github.com/mozilla-b2g/gaia/commit/daf91971902cd8e70e6cc5616159260cfcfce7c5

Comment 34

[Jason Smith [:jsmith]]

Fabrice - Can we chat about the approval request here? While I get the code risk was called out as low risk here, I'm concerned that we're adding non-critical strings to the release in this patch. At this point of stabilization, while we do allow for strings in landings, they really need to be critical strings for a particular release (e.g. string change needed for a required 2.1 feature). This feature however isn't critical to 2.1 (it's a nice to have at best), so adding this feature into this release seems like it's more risk than it's value due to the localization concerns.

Comment 35

[[:fabrice] Fabrice Desré]

I hear you, and I maybe should have rejected this one. But we're not yet in a hard lock down this week.