Merge lp:~alecu/ubuntu-sso-client/keep-old-credentials into lp:ubuntu-sso-client
Proposed by
Alejandro J. Cura
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Natalia Bidart | ||||
Approved revision: | 577 | ||||
Merged at revision: | 576 | ||||
Proposed branch: | lp:~alecu/ubuntu-sso-client/keep-old-credentials | ||||
Merge into: | lp:ubuntu-sso-client | ||||
Diff against target: |
224 lines (+156/-27) 2 files modified
ubuntu_sso/keyring.py (+41/-27) ubuntu_sso/tests/test_keyring.py (+115/-0) |
||||
To merge this branch: | bzr merge lp:~alecu/ubuntu-sso-client/keep-old-credentials | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Natalia Bidart (community) | Approve | ||
Rodrigo Moya (community) | Approve | ||
Review via email:
|
Commit message
Use a proper dictionary with different attributes for each key so old keys are not overwritten nor deleted.
Description of the change
The gnome keyring needs to be passed a dictionary containing attributes to single out each key in a given keyring. An empty dictionary was being used instead, which lead to old keys being deleted.
To solve this issue we are using a proper dictionary with different attributes for each key.
To post a comment you must log in.
I think we should starting adding some test cases to this module. From a talk with alecu on IRC:
(04:21:22 PM) alecu: nessita, should I add info on how to run it to the merge proposal? item_delete_ sync is not called while doing a set_ubuntusso_attr
(04:22:03 PM) nessita: alecu: instructions would be nice
(04:22:28 PM) nessita: alecu: but also some unit tests. Could you please explain why is not possible to add unittests?
(04:24:09 PM) alecu: nessita, I can add unit tests, sure. But I can't find a way to add a unit test that checks if this problem is fixed, because checking it requires accessing the gnomekeyring
(04:25:13 PM) nessita: alecu: maybe I don't fully understand this problem :-), so, could you please explain?
(04:34:18 PM) alecu: nessita, the problem was that when adding new credentials to the keyring, old credentials with the same set of attributes (in this case, an empty dict of attributes) were being erased. The way to check if old credentials are kept is by adding a new sets of credentials, and checking that they are still there after adding a second new set of credentials.
(04:38:03 PM) nessita: alecu: I don't see why that can't be tested with a mock. We can create a faked gnome keyring class and add two set of credentials, and assert the both are there
(04:38:30 PM) nessita: and this mock will behave like the real gnome keyirng when the set of attrs is the same (the emtpy dict)
(04:39:02 PM) nessita: we can also assert that the gnomekeyring.
(04:39:24 PM) nessita: that last check I think is the most important one, if I understood correctly