1221594 - Create LoginsDetailViewController to display the details of a single login as well as options for editing/deleting

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Stephan Leroux [:sleroux]]

Attachment: Screenshot 2015-11-04 11.31.10.png

When selecting a login from a list of logins, we want to display the details for that login including title, username, password, and url. Additionally, we want to include options for the user to edit the various login fields as well as delete the login. See attached screenshot for additional info.

Comment 1

[Stephan Leroux [:sleroux]]

In the designs, we have a footer with the label 'Last modified XXXX' where XXX is the last modified timestamp. Looking at our logins code we have a LoginUsageData protocol with the following:

public protocol LoginUsageData {
    var timesUsed: Int { get set }
    var timeCreated: MicrosecondTimestamp { get set }
    var timeLastUsed: MicrosecondTimestamp { get set }
    var timePasswordChanged: MicrosecondTimestamp { get set }
}

Doesn't look like we capture last modified but we do know the last time the password was changed which is probably equally useful. One issue is that for local records this is always set to now. Is it possible to know the last time a local login's password was changed or maybe I'm looking at the wrong place? 

My WIP commit for this is here: https://github.com/mozilla/firefox-ios/commit/6a2e93a0fc82932886a1b20a1813593369be6345

Comment 2

[Stephan Leroux [:sleroux]]

Also, we should probably show an error dialog in the case the domain they modified the login to is invalid. I can land some basic copy today before the string freeze. Something like:

'Invalid website. Make sure that the website your login is for is a valid URL.'

Comment 3

[Stephan Leroux [:sleroux]]

I wonder if we should also warn the user if they entered a blank username/password.

Comment 4

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1380

Comment 5

[Stephan Leroux [:sleroux]]

Clearing copy flags because editing will be handled as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1221595

Comment 6

[Emily Toop (:fluffyemily)]

Comment on attachment 8700734 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1380

Fix the failing tests and we're good to go.

Comment 7

[Stephan Leroux [:sleroux]]

Thanks for the quick review! I've fixed the tests and landed it.

master b9b53299f484fd91659c6ce10b6042cad600ecd3

Comment 8

[Stephan Leroux [:sleroux]]

Going to reopen until dependency bugs are also finished.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Stephan Leroux [:sleroux] from comment #1)

> Doesn't look like we capture last modified but we do know the last time the
> password was changed which is probably equally useful. One issue is that for
> local records this is always set to now. Is it possible to know the last
> time a local login's password was changed or maybe I'm looking at the wrong
> place?

The database stores both last modified (see LocalLogin) and time changed in loginsL. 

getUsageDataForLoginByGUID returns timePasswordChanged values for both local and remote records. updateLoginByGUID and addLogin both set timePasswordChanged correctly. The field is there and should be correct.

The only thing missing is populating the fields correctly in the Login instances when querying -- the search APIs (getLoginsForProtectionSpace, searchLoginsWithQuery, etc.) return LoginData instances rather than Logins, so those are stubbed out.

Comment 10

[Stephan Leroux [:sleroux]]

To clarify,

Local and remote logins have timePasswordChanged.
Local logins have local_modified when the login was last 'modified'

Some questions:

1. How come we have a modified timestamp only for local logins and not remote records?
2. Would using the timePasswordChange suffice for the 'Last Modifed: xxxx' label? This doesn't really mean 'last modified' though but if there is no way to know when a remote login was 'modified' I don't see what else we can do?
3. Depending on (1, 2), how would one bubble up the last modified time using a LoginData instance? Only thing I can think of is we'd have to modified the UNION ALL query to include rows that are not in both tables and do some massaging to map a column into something that makes sense for both. Again, this doesn't really make sense either since timePasswordChanged != lastModifed.

Comment 11

[Richard Newman [:rnewman]]

> 1. How come we have a modified timestamp only for local logins and not
> remote records?

Remote records have server_modified, which is a server clock value.

Local records have local_modified, which is a client clock values.

When a server record is locally modified, that password has *two* modification times: the timestamp of the last server record, and the timestamp of the last local modification that overlays the server record.

Those timestamps are used to reconcile conflicts.

Non-Sync code shouldn't be using *either* modification time -- they're not part of the value itself, and they don't appear in the Login object that frontend code gets to see.


> 2. Would using the timePasswordChange suffice for the 'Last Modifed: xxxx'
> label? This doesn't really mean 'last modified' though but if there is no
> way to know when a remote login was 'modified' I don't see what else we can
> do?

timePasswordChanged is the moment in time, from *a* client clock (not necessarily ours!), at which the password itself was changed.

It should always be later than or equal to timeCreated, and earlier than or equal to timeLastUsed.

timeLastUsed is optional.

You should use timePasswordChanged to indicate Last Modified.


> 3. Depending on (1, 2), how would one bubble up the last modified time using
> a LoginData instance?

Not last modified, time changed, but: most places that return LoginData should instead return Login, which is LoginData + LoginUsageData.

Comment 12

[Stephan Leroux [:sleroux]]

Resolved including password last modified fix.