Bugzilla – Full Text Bug Listing
| Summary: | Fetch from remote now always prompts for SSH information | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Ken Walker <ken_walker> | ||||||
| Component: | Git | Assignee: | Szymon Brandys <Szymon.Brandys> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | malgorzata.tomczyk, susan, Szymon.Brandys | ||||||
| Version: | 0.4 | Flags: | malgorzata.tomczyk:
review+
ken_walker: review+ |
||||||
| Target Milestone: | 0.4 RC3 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | 365982 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Ken Walker
just hitting the checkmark does fetch the remote We collect credentials via slideouts which do not know whether the repo needs credentials or not. Either we need to disable it and go back to the old dialog (doable for .4) or let the command control when the slideout should be opened (Susan?). letting the command control the slideout content is bug 365982 which I was hoping to defer to 0.5 I think there is a middle ground where we could keep the slideout. The code that associates the repo with the command (renderCommands) could check the repo and set the required parameters on the fly. Admittedly this is a hack. bug 365982 is basically the same thing, but it's the command framework triggering the call. Discussed with Szymon, i'll investigate a contained fix for bug 365982 I fixed the dependent bug. I think it may still be awkward given the structure of the git commands (which is why we had bug 370255). From what i recall, you had to try to authenticate before you really knew what you might have in the way of credentials. In bug 365982 you said that the parameters have to be collected during the callback, but of course parameters are something you specify that are a "prereq" to your callback being called. The callback needs to assume the parameters are already there. So...if the default values and/or the parameters themselves depend on something like the repository, then you need a hook to know what repository is involved before the callback is called. This is what I added in bug 365982. 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... If the problem is that the only way to find out what credentials you need is to actually ask the server to do something, then you need to have some kind of server-side request that asks the server to validate credentials. You could call that request in the parameter callback (but please do not return a deferred as we don't assume most clients have such complex requirements...if you must call the server to determine what parameters are needed then wait for the response... Created attachment 211273 [details]
Proposal v1
This patch changes just the Fetch command.
Gosia what do you think? (In reply to comment #7) > Gosia what do you think? 1. Errors are not displayed 2. Even if we know user name we still ask for it 3. handleProgressServiceResponse2 has errCallback - name suggests that it will be called on error, but it's called only when it's ssh error 4. Git log is not refreshed after successful Fetch, this is something we have in logs: "could not find table row gitapiremoteoriginmasterfile3". Force fetch that wasn't changed doesn't have this error. (In reply to comment #8) > 1. Errors are not displayed Hmm. Right. They are not displayed if you switched to the old dialog using 'More'. I missed that. > 2. Even if we know user name we still ask for it There is a separate bug for that in the slideout. For the old dialog, the same issue as in 1) > 3. handleProgressServiceResponse2 has errCallback - name suggests that it will > be called on error, but it's called only when it's ssh error Sure. The name can be changed. > 4. Git log is not refreshed after successful Fetch, this is something we have in > logs: "could not find table row gitapiremoteoriginmasterfile3". Force fetch that > wasn't changed doesn't have this error. Please raise a separate bug for that. It works this way without the patch too. Thanks. (In reply to comment #9) > > 4. Git log is not refreshed after successful Fetch, this is something we have in > > logs: "could not find table row gitapiremoteoriginmasterfile3". Force fetch that > > wasn't changed doesn't have this error. > Please raise a separate bug for that. It works this way without the patch too. > I opened Bug 372040, but my comment is still valid. Without your patch the error is still shown in logs, but the log *does* refresh. It seems that this error is not related to the not refreshing problem. I reviewed code you pushed to branch bug370978, those are my comments: * pull command doesn't use slideout at all (it never did). I think we should unify that, if it's too late now we could wait until 0.5, but we should do it. * The slideout still asks for the user name, but I understand that it's Bug 370255 * You added handleProgressServiceResponse2 function in addition to currently used handleProgressServiceResponse. I understand that you wanted to limit the number of changes because it's late in development cycle, but for 0.5 we should merge them * (Nitpicking) There are some places where you added 4 empty lines and nothing more. * While testing I also came across Bug 371882, but it's reproducable without your patch as well (In reply to comment #11) > I reviewed code you pushed to branch bug370978, those are my comments: > * pull command doesn't use slideout at all (it never did). I think we should > unify that, if it's too late now we could wait until 0.5, but we should do it. I would say it is too late. I decided to fix places where Susan already added slideouts. I would wait wit other places till .5 and Bug 372127 is fixed. > * The slideout still asks for the user name, but I understand that it's Bug > 370255 Yup. > * You added handleProgressServiceResponse2 function in addition to currently > used handleProgressServiceResponse. I understand that you wanted to limit the > number of changes because it's late in development cycle, but for 0.5 we should > merge them Yup. > * (Nitpicking) There are some places where you added 4 empty lines and nothing > more. Really? I will fix that before pushing. > * While testing I also came across Bug 371882, but it's reproducable without > your patch as well Right. You may review the fix at branch bug370978 on orion.client repo. As project lead I +1 this for RC3 Fixed with 5dbac688df2e1fe958c541704d94df5544a589fc. |