796393 - [email] duplicate accounts can be created

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect, P3)

Description

[GH to BZ]

[GitHub issue by nhirata on 2012-07-16T22:11:37Z, https://github.com/mozilla-b2g/gaia/issues/2476]
## Environment
* Otoro phone, Gaia build 2012-07-16
* Gaia commit: 0d3ee74b02bc01f88e14e5c9f068fff91d4bda01
* Gecko commit : 422ac3fb4ea6550c3ed55f29e8fac6f43d13c682

## Repro
1. launch email webapp for the first time
2. Select Fake Email
3. click next
4. click on add another account
5. click Fake email
6. click next

## Expected:
* error message : email already is added

## Actual:
* All Done! ; checking in  ||| -> Gear , you can see multiple fake accounts added.
* No way to delete the accounts added.

Comment 1

[GH to BZ]

[GitHub comment by asutherland on 2012-07-16T22:34:53Z]
If you long hold on the accounts in the account list, it should pop up a menu which has a delete option.  On b2g-desktop/firefox, the right mouse button also accomplishes the same task.

Note that there is a bug about long hold generating a click in a way that is fairly annoying so as to make the long hold barely usable: https://bugzilla.mozilla.org/show_bug.cgi?id=766813

Comment 2

[GH to BZ]

[GitHub comment by nhirata on 2012-07-16T22:49:04Z]
Ah... I missed that.  thanks.

Comment 3

[Mike Connor [:mconnor]]

QA: please repro with a real account, and renominate with updated STR if it is believed this is a blocker-grade issue.

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: screenshot

screenshot.  Still can get email, just confusing.  I suppose it's not a real blocker though.

Comment 5

[ayman maat :maat]

I am still able to create duplicate accounts.

**PATH**
1) first account created
1.1) open email account
1.2) select email provided
1.3) enter user name, email address password
1.4) select next

2) second account
2.1) select 'add another account'
2.2) select the same email provider as in 1.2)
2.3) enter the same user name, email address password as in 1.3)
2.4) select next

**EXPECTED**
message informing user that account is already set up

**ACTUAL**
duplicate email account created

It has been over two and a half months since this bug was first raised. Could I ask if we have a plan for when this will be addressed?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to ayman maat :maat from comment #5)
> I am still able to create duplicate accounts.

Unless the bug got marked resolved, there wouldn't really be a reason to suspect otherwise.
 
> It has been over two and a half months since this bug was first raised.
> Could I ask if we have a plan for when this will be addressed?

B2G bugs get worked in order of priority, this one is an extremely low priority since the user has to explicitly create duplicate accounts by either entering duplicate credentials in their initial setup session, or go to the account list, ignore the already listed account, and then add it again.

Comment 7

[ayman maat :maat]

> B2G bugs get worked in order of priority, this one is an extremely low
> priority since the user has to explicitly create duplicate accounts by
> either entering duplicate credentials in their initial setup session, or go
> to the account list, ignore the already listed account, and then add it
> again.

From a UX PoV i agree that this is extremely low priority compared to the other bugs associated to the email app. Thanks for the update as to where we are with this bug Andrew

Comment 8

[Natalya Kot [:nkot]]

issue also occurs in Unagi devices, Biuld ID:20130112070202

Comment 10

[Leo]

Attachment: Pointer to Pull request

We have verified the new account that user wants to create present in the already configured accounts. If it present showing notification error saying "The account is already configured"
There is an String added in locales [setup-error-account-already-exists=This account is already configured], we will create a new bugzilla issue for that to generate the string for all other languages.

Please review and let us know your comments
Thanks.

Comment 11

[Leo]

@Andrew

As part of this fix, we added a new string [setup-error-account-already-exists=This account is already configured]. Please provide information to make it apply in the code and get it translated for all the languages.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 719938 [details]
Pointer to Pull request

Thanks for the patch.  I believe that the right place to do this is in the e-mail back-end.  Its repo is at:
https://github.com/mozilla-b2g/gaia-email-libs-and-more

You would want to modify tryToCreateAccount in the MailUniverse class:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/mailuniverse.js#L663
The check can be synchronous inside that function.

And add the new error code you have created to the documentation tryToCreateAccount on the MailAPI class:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/mailapi.js#L1519

You would want to keep the string you added to apps/email/locales/email.en-US.properties from this pull request and the mapping in SETUP_ERROR_L10N_ID_MAP.

To test your changes after you've made then in gaia-email-libs-and-more, you would run "make install-into-gaia" from the root of the gaia-email-libs-and-more directory.  Make sure you have created all the symlinks as documented in the README.md

In terms of unit tests, the first test case in:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/test/unit/test_account_logic.js
is going to break because it intentionally (for reasons of laziness) creates multiple accounts that are effectively duplicates.

I believe our ActiveSync fake-server accepts any credentials, so the thing to do for that unit test wold probably be to:

- modify the test to use different hard-coded credentials for each account we are intentionally creating.  The test case will break for IMAP ("make post-imap-tests"), but should be okay for ActiveSync ("make post-activesync-tests").  To just run the one test, you can do "make post-one-activesync-test SOLO_FILE=test_account_logic.js"

- intentionally then still try and create a duplicate account, and verify that we get a failure. test_account_create_unit.js intentionally has some failures, which might be a good place to look.

Please keep in mind that to see if a test actually succeeded currently, you do need to check it out in the ArbPL UI.

Our unit test infrastructure is a little weird, so if you are having trouble getting the tests to run, please feel free to ask me questions.  And if it's really not working out for you, I can do the test fix-up for you as part of my review of your revised patch.


So, to be clear, we should end up with 2 pull requests with the revised patch: 1 in gaia-email-libs-and-more, and 1 in gaia.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

(In reply to leo.bugzilla.gaia from comment #11)
> As part of this fix, we added a new string
> [setup-error-account-already-exists=This account is already configured].
> Please provide information to make it apply in the code and get it
> translated for all the languages.

So, all we need to do is do what you have done, and the string will eventually get localized by localizers when the automation notices there is a missing string.  This is okay for gaia/master at least.  If you want this uplifted to v1-train for v1.1.0, I think we need to make sure release management knows that there is an additional string that will need to be localized on the branch.  I assume if the functionality is sufficiently desired, they will be okay with the extra string.

Comment 14

[Leo]

Hi Andrew,
I worked on it and modified the code at gaia-email-libs-and-more and gaia also.

I want to know , should gaia patch include the ext - js modifications which were generated using "make install-into-gaia "? or only setup-cards.js is enough?
Please let me know.

Thanks
Leo

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

It's easier to review the changes to gaia if the "make install-into-gaia" changes aren't included in the pull request, so that's slightly preferred.

When it's time to actually land the changes (after the gaia-email-libs-and-more pull request has been reviewed and landed), the changes should of course be folded into the patch.

If you need to make it easy for people than other reviewers to try running with your patch, that's an acceptable reason to include the changes in your patch, however.  (While it's easy for me to do "make install-into-gaia" to test, I wouldn't expect us to make QA people do it.)

Comment 16

[Leo]

Attachment: Pointer to Gaia Pull Request

Pointer to Gaia Pull Request
This has only new Error type add for SETUP_ERROR_L10N_ID_MAP and English locale string.
check the next pointer for the Patch of gaia-email-libs-and-more, 
Please review
Thanks,
Leo

Comment 17

[Leo]

Attachment: Pointer to Email Lib Pull Request

This is the pointer for gaia-email-libs-and-more.
I tried to do Unit Tests but could not succeed. I had given symlinks properly but both imap and activesync tests break. Did not understand completely.Please check at your end.

Please review 
Thanks,
Leo

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

The patch looks good by initial inspection; it will take me a few days, but I should be able to fix the current test that breaks and write up a new test and land this soon.  Thanks very much!

Comment 19

[Leo]

Thanks Andrew.

Comment 20

[Leo]

Hi Andrew 

Please let us know status of this patch.

Thanks
Leo

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

I'm sorry about the delay; we've been focusing on performance improvements for e-mail so I haven't been able to allocate the time to dealing with the unit test fallout of the change.  We should ideally be able to address that soon.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

There's nothing to indicate that this should be a blocker - comment 7 states this is low priotity, tracking instead.

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 721683 [details]
Pointer to Email Lib Pull Request

I am very sorry I have been unable to get to fixing the test for this bug.  As you may be aware, there have been a lot of high priority fixes going on.  For clarity, I'm going to clear the review flag on this bug because we cannot land this fix as-is because it breaks several unit tests, albeit as a byproduct of those (reasonably important) tests taking shortcuts by creating duplicate accounts.  The logic for duplicate account detection and reporting continues to be sound, as previously noted.

It would be great if you could take another dive at getting our unit tests to run, as we have improved the situation greatly.  However, if you can't, feel free to un-assign the bug from yourself and hopefully we can get to this bug per the priority list.

Comment 24

[Leo]

Due to other issues or work, not able to check right now about the unit test.
So it should not be on hold, due to which I am assigning it to default.

Comment 25

[Jim Porter (:squib)]

Just a quick update on this: I've got all the tests passing for the backend part of the patch, and now all I need to do is write a new test to ensure we can't create duplicate accounts under normal circumstances. That should be done early tomorrow.

Comment 26

[Jim Porter (:squib)]

Attachment: GELAM patch

Comment 27

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/198
        https://github.com/mozilla-b2g/gaia/pull/9550

Comment 28

[Jim Porter (:squib)]

Comment on attachment 745269 [details]
GELAM patch

r+ over IRC

Comment 29

[Stephany Wilkes]

It does not seem like UX is needed any longer for this particular bug. Please re-add as necessary.

Comment 30

[Andrew Sutherland [:asuth] (he/him)]

Note that there was a bug in this fix, so if this patch gets uplifted, the fix on bug 870042 *must be uplifted as well*.

Comment 31

[Jim Porter (:squib)]

Comment on attachment 745269 [details]
GELAM patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Users will be able to create duplicate mail accounts, which isn't ideal.
Testing completed: Unit tests in GELAM, and manual testing on the phone
Risk to taking this patch (and alternatives if risky): This will also need bug 870042 to land at the same time. This should be low-risk though, and has good test coverage.
String or UUID changes made by this patch: One string added (see the Gaia PR) to indicate that an account with the specified email address already exists

Comment 32

[Ryan VanderMeulen [:RyanVM]]

John, can you uplift to v1-train please?

Comment 33

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: f5e454a