Bug 370255

Comment 1 Szymon Brandys 2012-02-01 03:52:14 EST

Gosia, could you help Susan here?

Comment 2 Susan McCourt 2012-02-08 10:21:57 EST

We never assigned this.
Gosia, can you help?
This is more apparent now that we use slideouts in the sections on the repo page.

Comment 3 Susan McCourt 2012-02-09 00:52:53 EST

You may have more flexibility for a solution now that bug 365982 is implemented.

I would hope you could check the credentials in the new parameter callback, figure out if you authenticated, decide what parameters are needed, and then the callback will be called with any missing credentials in the parameters object (after the slideout is finished).

Then do the real work in the main callback.  If the problem is that you can only check credentials by trying to do something, then I think you need some innocuous server request that does the check so that you know what's needed.  So...

Where you had:
var gitAuthParameters = new mCommands.ParametersDescription([new mCommands.CommandParameter("sshuser", "text", "SSH User Name:"), new mCommands.CommandParameter("sshpassword", "password", "SSH Password:")], true);

you can now say
var gitAuthParameters = new mCommands.ParametersDescription([new mCommands.CommandParameter("sshuser", "text", "SSH User Name:"), new mCommands.CommandParameter("sshpassword", "password", "SSH Password:")], true, function(data) {
   if (data.items.... means no need to authenticate) {
     return null;
   } else if (data.items.....means we know the user but not password) {
     return [new mCommands.CommandParameter("sshpassword", password", "SSH Password:"];
   } else { // we really need both
     reutrn [new mCommands.CommandParameter("sshuser", "text", "SSH User Name:"), new mCommands.CommandParameter("sshpassword", "password", "SSH Password:")]
   }
});

If you need to hang some kind of state on the parameters so that the callback can get to it, you could hang some properties on the "data" in the callback.  That is the same data that will be sent to the callback.  This isn't really documented, but it sounds like your case is a bit awkward, where you have to try to authenticate before you can know what you need.  So if that try will get some state that you need, you can store it there. 

Such as function(data)
   if (data.items...means no authentication) {
     data.secretStashOfCachedInfo = ...;
     return null;  // no slideout
   } else {...

Then in the callback
  callback: function(data) {
    mySecretStuff = data.secretStashOfCachedInfo...

Comment 4 Susan McCourt 2012-02-09 01:06:33 EST

I should have mentioned that the parameter callback is assumed to be synchronous.  It is intended for cases where the callback item needs to be checked in order to provide a good default parameter value.

Most commands would have patterns that access the callback item (data.items) and then figure out what's appropriate, such as:
function(data) {
  return [new mCommands.CommandParameter("name", "text", "Name:", data.items.Name)];
}

If you must call the server during this call, please wait for the response before providing the parameters...

Comment 5 Malgorzata Janczarska 2012-02-09 11:06:16 EST

Let me sum up the problems we have
1. Before we start fetching (or cloning, or pushing...) we don't know if we need any credentials or not
2. The only way to find out if we need credentials is to try to fetch
3. Fetching *can not* be run synchronously. Those are long running operations that can go for 30 minutes or more.
4. Fetch may succeed without credentials or return "Auth fail". We won't get more information from it, don't know whether we need a password or a passphrase or if our password was just incorrect.
5. If user name is a part of URL we can't even change it, the inputed user name will be ignored and always the one from URL will be used
6. User may input the wrong credentials and we have to ask him for credentials again

How would I ideally see it:
1. When user clicks on fetch we do the normal fetch without credentials. It's asynchronous, we should do it in callback
2. If credentials failed we use the slide out
- we check for username in URL, if it's there we don't display the name field, only password
- we don't put private key and passphrase in the slide out, because it's too big
- under "more" we put the old credentials dialog where user may also input the private key
3. On OK we send the credentials inputed by the user and retry fetch
4. If fetch fails again we can
a) open the slideout again?
b) open the old credentials dialog?
c) open the slideout if user previously used in for credentials, open old dialog if previously user used "more"?
5. Retry

For this we would have to use the commandService.openParameterCollector the same way as "Serach... Replace" does, as an input dialog.

Comment 6 Susan McCourt 2012-02-09 11:19:13 EST

this makes sense (opening the slideout manually).
As far as what to do on failure...I like (c) if it is easy to keep track

>c) open the slideout if user previously used in for credentials, open old
dialog if previously user used "more"?

I realize we have a general issue with different input collection methods.  That is bug 366421.  I think the best I'll do in 0.4 is just make sure we close slideouts when another command runs.  The "holy grail" from my point of view is eventually rehost our dialogs in a slideout, so that pushing "more" would capture the additional information in line.  Or at least I'd like to try some more content heavy slideouts to see how they play.  But not now.

Comment 7 Simon Kaegi 2012-02-16 15:11:14 EST

Szymon this is fine to commit right after RC2 is declared -- e.g. for RC3. Please still get another to review the change.

Comment 8 Szymon Brandys 2012-02-22 06:48:43 EST

Created attachment 211395 [details]
Fix v1

Comment 9 Szymon Brandys 2012-02-22 06:49:13 EST

Gosia, please review.

Comment 10 Malgorzata Janczarska 2012-02-22 08:03:47 EST

Very nice. Works fine.

Comment 11 Szymon Brandys 2012-02-22 08:24:28 EST

Fixed with bc28ce8d2769cffa65dfc7ad2b3a9b77be10e50e.