Show warning message when trying to create SU with existing name

Show warning message when trying to create SU with existing name

Disallow creating a new supervised user if there is already existing SU user with the same name locally. If there are multiple supervised users with the same name, then disallow creating when all of them are imported locally. If at least one of them is not, show the import dialog.

BUG=557445
Committed: https://crrev.com/4688e7512edad31d7f0c2cbafe4bb2e4d73c47a7
Cr-Commit-Position: refs/heads/master@{#366364}

[atanasova, 2015-12-10 10:57:54]

Description was changed from

==========
Show warning message when trying to create SU with existing name

Disallow creating a new supervised user if there is already existing SU user
with the same name locally. If there are multiple supervised users with the same
name, then disallow creating when all of them are imported locally. If at least
one of them is not, show the import dialog.

BUG=557445
==========

to

==========
Show warning message when trying to create SU with existing name

Disallow creating a new supervised user if there is already existing SU user
with the same name locally. If there are multiple supervised users with the same
name, then disallow creating when all of them are imported locally. If at least
one of them is not, show the import dialog.

BUG=557445
==========

[atanasova, 2015-12-10 10:57:55]

atanasova@chromium.org changed reviewers:
+ treib@chromium.org

[atanasova, 2015-12-10 10:58:50]

I have not added unit tests since I am not sure if we have any or their
location. If I need to add something, please tell me where I can do that.

[Marc Treib, 2015-12-10 11:36:41]

treib@chromium.org changed reviewers:
+ pam@chromium.org

[Marc Treib, 2015-12-10 11:36:41]

Looks good! Just some minor comments below, and the option to reuse a similar
string from ChromeOS. +pam to get her opinion on the string reuse. (And,
eventually, for owners approval :) )

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12933: +        <message
name="IDS_PROFILES_CREATE_EXISTING_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY"
desc="Message shown when the user enters the name of a supervised user that we
already has locally.">
nit: That we already haVE

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12934: +          Looks like you have already
imported a supervised user with this name.
Hm, we haven't necessarily imported it, we might just have created it here.
Maybe just "Looks like you already have a supervised user with this name."?

Also, I just noticed that on ChromeOS we already have a string for this case:
IDS_CREATE_SUPERVISED_USER_CREATE_USERNAME_ALREADY_EXISTS
"Whoops, this name is already in use!"
Maybe we should reuse that one here, or maybe not, since the "style" is somewhat
different... let's see what Pam thinks.

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:385: var
allOnCurrentDevice = supervisedUsers[i].onCurrentDevice;
I think this is worth a comment: You're handling the case when there already are
multiple SUs with that name, which "shouldn't" happen (for small values of
"shouldn't"). Took me a moment to understand this :)

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:96:
IDS_PROFILES_CREATE_EXISTING_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY },
nit: "exist" is in there twice; can we remove one and avoid the overlong line?
:)
e.g.
IDS_PROFILES_CREATE_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY
and then also rename the other message to
IDS_PROFILES_CREATE_LEGACY_SUPERVISED_USER_ERROR_EXISTS_REMOTELY
to make the difference clear.

(Or ignore all this if we decide to reuse the CrOS string...)

[Pam (message me for reviews), 2015-12-15 14:31:37]

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12934: +          Looks like you have already
imported a supervised user with this name.
On 2015/12/10 11:36:41, Marc Treib wrote:
> Hm, we haven't necessarily imported it, we might just have created it here.
> Maybe just "Looks like you already have a supervised user with this name."?
> 
> Also, I just noticed that on ChromeOS we already have a string for this case:
> IDS_CREATE_SUPERVISED_USER_CREATE_USERNAME_ALREADY_EXISTS
> "Whoops, this name is already in use!"
> Maybe we should reuse that one here, or maybe not, since the "style" is
somewhat
> different... let's see what Pam thinks.

"Looks like you already have a supervised user with this name." sounds good, at
least once the functionality is changed to disallow all dupes rather than only
local ones. Otherwise we probably want to add "...on this device" or similar.

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:385: var
allOnCurrentDevice = supervisedUsers[i].onCurrentDevice;
On 2015/12/10 11:36:41, Marc Treib wrote:
> I think this is worth a comment: You're handling the case when there already
are
> multiple SUs with that name, which "shouldn't" happen (for small values of
> "shouldn't"). Took me a moment to understand this :)

It can happen if the first SU with that name hasn't been imported onto this
device.

That said, we don't want to support two SUs with the same name at all. There
might be some existing duplicates, though, from the time this bug has existed.
So we should disallow creating a new SU with a name that exists for any SU (not
just on this device) -- but still allow importing any SU that the current user
already has, even if it duplicates a name.

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:96:
IDS_PROFILES_CREATE_EXISTING_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY },
On 2015/12/10 11:36:41, Marc Treib wrote:
> nit: "exist" is in there twice; can we remove one and avoid the overlong line?
> :)
> e.g.
> IDS_PROFILES_CREATE_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY
> and then also rename the other message to
> IDS_PROFILES_CREATE_LEGACY_SUPERVISED_USER_ERROR_EXISTS_REMOTELY
> to make the difference clear.
> 
> (Or ignore all this if we decide to reuse the CrOS string...)

Or a new string with a more suitable tone but still covering both cases.

[atanasova, 2015-12-15 17:57:36]

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12933: +        <message
name="IDS_PROFILES_CREATE_EXISTING_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY"
desc="Message shown when the user enters the name of a supervised user that we
already has locally.">
On 2015/12/10 11:36:41, Marc Treib wrote:
> nit: That we already haVE

Done.

https://codereview.chromium.org/1506353007/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12934: +          Looks like you have already
imported a supervised user with this name.
On 2015/12/15 14:31:36, Pam (also PM for reviews) wrote:
> On 2015/12/10 11:36:41, Marc Treib wrote:
> > Hm, we haven't necessarily imported it, we might just have created it here.
> > Maybe just "Looks like you already have a supervised user with this name."?
> > 
> > Also, I just noticed that on ChromeOS we already have a string for this
case:
> > IDS_CREATE_SUPERVISED_USER_CREATE_USERNAME_ALREADY_EXISTS
> > "Whoops, this name is already in use!"
> > Maybe we should reuse that one here, or maybe not, since the "style" is
> somewhat
> > different... let's see what Pam thinks.
> 
> "Looks like you already have a supervised user with this name." sounds good,
at
> least once the functionality is changed to disallow all dupes rather than only
> local ones. Otherwise we probably want to add "...on this device" or similar.

Changed it to Pam's suggestion without the "on this device". I think that since
users with duplicate names could have been created, we cannot assume if all of
them are on the device or not.

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:385: var
allOnCurrentDevice = supervisedUsers[i].onCurrentDevice;
On 2015/12/15 14:31:37, Pam (also PM for reviews) wrote:
> On 2015/12/10 11:36:41, Marc Treib wrote:
> > I think this is worth a comment: You're handling the case when there already
> are
> > multiple SUs with that name, which "shouldn't" happen (for small values of
> > "shouldn't"). Took me a moment to understand this :)
> 
> It can happen if the first SU with that name hasn't been imported onto this
> device.
> 
> That said, we don't want to support two SUs with the same name at all. There
> might be some existing duplicates, though, from the time this bug has existed.
> So we should disallow creating a new SU with a name that exists for any SU
(not
> just on this device) -- but still allow importing any SU that the current user
> already has, even if it duplicates a name.

I added a comment to explain this case. It can happen if the user has already
created supervised users with the same name. That is why we need to check, in
order to allow import if not all are synced on the device.

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/1506353007/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:96:
IDS_PROFILES_CREATE_EXISTING_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY },
On 2015/12/15 14:31:37, Pam (also PM for reviews) wrote:
> On 2015/12/10 11:36:41, Marc Treib wrote:
> > nit: "exist" is in there twice; can we remove one and avoid the overlong
line?
> > :)
> > e.g.
> > IDS_PROFILES_CREATE_LEGACY_SUPERVISED_USER_ERROR_EXISTS_LOCALLY
> > and then also rename the other message to
> > IDS_PROFILES_CREATE_LEGACY_SUPERVISED_USER_ERROR_EXISTS_REMOTELY
> > to make the difference clear.
> > 
> > (Or ignore all this if we decide to reuse the CrOS string...)
> 
> Or a new string with a more suitable tone but still covering both cases.

The second type of string is useful since it contains a link to the import
option.

[Pam (message me for reviews), 2015-12-15 22:20:13]

For documentation purposes, please link screenshots showing the user flows for
the various situations.

https://codereview.chromium.org/1506353007/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:385: // Handling the
case when multiple SU user with the same
nit:
user with the same name are created
->
users with the same name exist

Also, please add a note about how this can happen; e.g.:
A bug allowed duplicate SU names for a time, so we need to handle this case.

[Pam (message me for reviews), 2015-12-15 22:20:51]

On 2015/12/15 22:20:13, Pam (also PM for reviews) wrote:
> For documentation purposes, please link screenshots showing the user flows for
> the various situations.
> 
>
https://codereview.chromium.org/1506353007/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/options/manage_profile_overlay.js (right):
> 
>
https://codereview.chromium.org/1506353007/diff/40001/chrome/browser/resource...
> chrome/browser/resources/options/manage_profile_overlay.js:385: // Handling
the
> case when multiple SU user with the same
> nit:
> user with the same name are created
> ->
> users with the same name exist
> 
> Also, please add a note about how this can happen; e.g.:
> A bug allowed duplicate SU names for a time, so we need to handle this case.

Also also, no need to wait overnight for my LG-TM once Marc's given his.

[atanasova, 2015-12-16 09:21:14]

https://codereview.chromium.org/1506353007/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:385: // Handling the
case when multiple SU user with the same
On 2015/12/15 22:20:13, Pam (also PM for reviews) wrote:
> nit:
> user with the same name are created
> ->
> users with the same name exist
> 
> Also, please add a note about how this can happen; e.g.:
> A bug allowed duplicate SU names for a time, so we need to handle this case.

Done.

[Marc Treib, 2015-12-16 09:26:54]

LGTM! (We'll still have to wait for Pam though, or add someone else, since I
don't own these files...)

https://codereview.chromium.org/1506353007/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:382: if
(supervisedUsers[i].name == newName) {
optional nit: You could say
if (supervisedUsers[i].name != newName) continue;
now, and save one level of indentation :)

[atanasova, 2015-12-16 14:35:37]

https://codereview.chromium.org/1506353007/diff/60001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/60001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:382: if
(supervisedUsers[i].name == newName) {
On 2015/12/16 09:26:54, Marc Treib wrote:
> optional nit: You could say
> if (supervisedUsers[i].name != newName) continue;
> now, and save one level of indentation :)

Done.

[Pam (message me for reviews), 2015-12-16 18:38:50]

LGTM with the small comment change described below.

https://codereview.chromium.org/1506353007/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:390: // creating SUs
with the same name.
This sentence could be misinterpreted as hypothetical rather than historical.
Adding a link to the bug would help.

https://crbug.com/557445

[atanasova, 2015-12-17 09:28:40]

https://codereview.chromium.org/1506353007/diff/80001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1506353007/diff/80001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:390: // creating SUs
with the same name.
On 2015/12/16 18:38:50, Pam (also PM for reviews) wrote:
> This sentence could be misinterpreted as hypothetical rather than historical.
> Adding a link to the bug would help.
> 
> https://crbug.com/557445

Done.

[atanasova, 2015-12-17 09:30:29]

The CQ bit was checked by atanasova@chromium.org

[atanasova, 2015-12-17 09:30:29]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
pam@chromium.org
Link to the patchset: https://codereview.chromium.org/1506353007/#ps100001
(title: "Adding link to the bug in the comment")

[commit-bot: I haz the power, 2015-12-17 09:31:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506353007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506353007/100001

[commit-bot: I haz the power, 2015-12-17 10:14:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 10:14:59]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[atanasova, 2015-12-21 09:56:11]

PTAL. Fixed the broken unit test and added a new one that tests for the bug
scenario

[Marc Treib, 2015-12-21 10:02:20]

Still LGTM

https://codereview.chromium.org/1506353007/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right):

https://codereview.chromium.org/1506353007/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:228: // Also add
the names 'Test' and 'SameName' to |existingProfileNames_| to
nit: Update comment ('RepeatingName')

[atanasova, 2015-12-21 10:11:53]

https://codereview.chromium.org/1506353007/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/manage_profile_browsertest.js (right):

https://codereview.chromium.org/1506353007/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/manage_profile_browsertest.js:228: // Also add
the names 'Test' and 'SameName' to |existingProfileNames_| to
On 2015/12/21 10:02:19, Marc Treib wrote:
> nit: Update comment ('RepeatingName')

Done.

[atanasova, 2015-12-21 10:11:59]

The CQ bit was checked by atanasova@chromium.org

[atanasova, 2015-12-21 10:11:59]

The patchset sent to the CQ was uploaded after l-g-t-m from pam@chromium.org,
treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1506353007/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-12-21 10:12:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506353007/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506353007/140001

[commit-bot: I haz the power, 2015-12-21 10:57:35]

Description was changed from

==========
Show warning message when trying to create SU with existing name

Disallow creating a new supervised user if there is already existing SU user
with the same name locally. If there are multiple supervised users with the same
name, then disallow creating when all of them are imported locally. If at least
one of them is not, show the import dialog.

BUG=557445
==========

to

==========
Show warning message when trying to create SU with existing name

Disallow creating a new supervised user if there is already existing SU user
with the same name locally. If there are multiple supervised users with the same
name, then disallow creating when all of them are imported locally. If at least
one of them is not, show the import dialog.

BUG=557445
==========

[commit-bot: I haz the power, 2015-12-21 10:57:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-21 10:58:13]

Description was changed from

==========
Show warning message when trying to create SU with existing name

Disallow creating a new supervised user if there is already existing SU user
with the same name locally. If there are multiple supervised users with the same
name, then disallow creating when all of them are imported locally. If at least
one of them is not, show the import dialog.

BUG=557445
==========

to

==========
Show warning message when trying to create SU with existing name

Disallow creating a new supervised user if there is already existing SU user
with the same name locally. If there are multiple supervised users with the same
name, then disallow creating when all of them are imported locally. If at least
one of them is not, show the import dialog.

BUG=557445

Committed: https://crrev.com/4688e7512edad31d7f0c2cbafe4bb2e4d73c47a7
Cr-Commit-Position: refs/heads/master@{#366364}
==========

[commit-bot: I haz the power, 2015-12-21 10:58:14]

Patchset 8 (id:??) landed as
https://crrev.com/4688e7512edad31d7f0c2cbafe4bb2e4d73c47a7
Cr-Commit-Position: refs/heads/master@{#366364}