Add public StartClone() method #325
Conversation
This is necessary so that flexbridge can supply required cloning information and start a repo download without direct/manual user input. This order will come from Fieldworks when launched from CMD or protocol handler with the proper arguments.
There was a problem hiding this comment.
Since this is an API change that we are interested in I think it deserves a version bump. Adding +semver:major in the commit comment will take care of that.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @josephmyers)
+semver:major
This gives us more flexibility (less security) for changing URI structure.
|
This change deserves a line in |
|
I'm a bit unclear. The commit comment on sillsdev/flexbridge#388 says that a dialog will be shown to confirm the download, and Will there be user interaction? (I assume you wouldn't want the user to edit anything, except perhaps the local project name.) |
| /// <param name="projectName">Name of the project to clone</param> | ||
| /// <param name="projectUri">URI where the project can be found</param> | ||
| /// </summary> | ||
| public void StartClone(string username, string password, string projectName, Uri projectUri) |
There was a problem hiding this comment.
I think the public method would make more sense as a static method to
- create a
GetCloneFromInternetModelfrom the arguments - create a
GetCloneFromInternetDialogfrom the model - StartClone(); with no arguments
|
@papeh there will be little to no user interaction. The idea is that a user clicks a button in Language Depot (in the browser) and FLEx opens and clones the project. There is a dialog that FLEx pops up (since it's what gets invoked by the protocol handler) which would prompt the user "Download Sena 3 from LanguageDepot.org?" as a security precaution (any website could invoke the protocol handler and we don't want FLEx to download stuff off the internet automatically without user permission). We could skip the dialog for authorized domains (such as LanguageDepot) but prompt for unexpected domains. I don't want to block unknown domains because it would be useful for testing, or if someone wanted to self host language depot. |
| /// <param name="username">Username for Mercurial authentication</param> | ||
| /// <param name="password">Password for Mercurial authentication</param> | ||
| /// <param name="projectFolder">The parent directory to put the clone in</param> | ||
| /// <param name="projectName">Name of the project to clone</param> |
There was a problem hiding this comment.
How about: "Name for the project on the local machine"?
When I read, "Project to clone," I think of the name on the server.
| /// <param name="projectName">Name of the project to clone</param> | ||
| /// <param name="projectUri">URI where the project can be found</param> | ||
| /// </summary> | ||
| public static GetCloneFromInternetDialog StartClone(string username, string password, string projectFolder, string projectName, Uri projectUri) |
There was a problem hiding this comment.
CloneResult would be a better return type; the client shouldn't have to dig figure out what happened. Since that would make this method handle the whole process, it should be renamed to DoClone
There was a problem hiding this comment.
I'm concerned about removing the Form return result, at first glance. FB needs to provide Application.Run() with something to keep from moving on immediately. Maybe it's just as simple as moving that call in here? I'll look into it. Edit: Everything hunky-dory!
| /// <param name="projectName">Name of the project to clone</param> | ||
| /// <param name="projectUri">URI where the project can be found</param> | ||
| /// </summary> | ||
| public static GetCloneFromInternetDialog StartClone(string username, string password, string projectFolder, string projectName, Uri projectUri) |
There was a problem hiding this comment.
Nitpicking: This dialog seems kind of a funny place for this method. I'd consider GetSharedProjectModel or even a new class.
There was a problem hiding this comment.
If you'd like me to move it, let me know where. (It doesn't matter too much to me.) I put it here, because it needs the model and dialog, both of which are present here. But its being static now makes separation easier
| @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
| ### Added | |||
|
|
|||
| - [SIL.Chorus.LibChorus] Add webm as additional audio file type | |||
| - [SIL.Chorus] Add ability to clone project without direct user interaction | |||
There was a problem hiding this comment.
"without user interaction": there's no indirect interaction, either.
There was a problem hiding this comment.
In my mind, the indirect user interaction is them using the wrapping software. They still have to do something to get the clone to happen.
|
@papeh @jasonleenaylor If there are things you'd like done to these PR's, especially comments that are minor or don't affect the code itself, feel free to do those. I'm not strongly opinionated on the vast majority of this. Doing so would avoid a not less than 24hr communication cycle time. But I also know you guys are super busy, so I'm more than happy to do them as soon as I can. Whatever I can do to help things along. (Sometimes it takes longer to make the comment than to make the change 😄) |
|
@papeh I did your first two comments and will wait for more guidance on the second two. Let me know what you think. Thanks |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Dismissed @hahn-kev and @papeh from 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hahn-kev, @jasonleenaylor, and @josephmyers)
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 276 at r3 (raw file):
Previously, papeh wrote…
I think the public method would make more sense as a static method to
- create a
GetCloneFromInternetModelfrom the arguments- create a
GetCloneFromInternetDialogfrom the model- StartClone(); with no arguments
Done
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 127 at r5 (raw file):
Application.Run(dialog); var cloneStatus = res == DialogResult.OK ? CloneStatus.Created : CloneStatus.NotCreated;
see GetSharedProjectModel for more robust CloneStatus parsing
There was a problem hiding this comment.
Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status:
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 277 at r4 (raw file):
Previously, josephmyers wrote…
If you'd like me to move it, let me know where. (It doesn't matter too much to me.) I put it here, because it needs the model and dialog, both of which are present here. But its being static now makes separation easier
I considered moving it but didn't bother:
- StartClone can stay private (instead of internal)
- DoClone is clearly specific to Internet clones (although that would be apparent from the args, as neither CH nor USB take credentials)
|
Looking good to me. Always a fan of making things simpler! |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hahn-kev, @jasonleenaylor, and @josephmyers)
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 306 at r6 (raw file):
return; UpdateDisplay(State.MakingClone); _model.SaveUserSettings();
Is it a problem that the passed bearer:token will overwrite the user's saved credentials, if any?
|
|
Previously, josephmyers wrote…the token can be used for any project the user has access to, so it should be ok. @jasonleenaylor we can talk about this next time we chat if you want. |
You certainly don't want to leave it just like this, but it's a start for an idea. You could add a flag to the method, or potentially interpret automatically from the existing inputs. Or you could change the ServerSettingsModel.
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @hahn-kev, @jasonleenaylor, and @papeh)
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 306 at r6 (raw file):
Previously, hahn-kev (Kevin Hahn) wrote…
the token can be used for any project the user has access to, so it should be ok. @jasonleenaylor we can talk about this next time we chat if you want.
Good catch, Hasso. I submitted a PoC hack to demonstrate reverting user/pass, though we'd want some sophistication if we went this route. Kevin also mentioned extending JWT lifecycles and various other tricks you can do with them.
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @hahn-kev, @jasonleenaylor, and @papeh)
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 306 at r6 (raw file):
Previously, josephmyers wrote…
Good catch, Hasso. I submitted a PoC hack to demonstrate reverting user/pass, though we'd want some sophistication if we went this route. Kevin also mentioned extending JWT lifecycles and various other tricks you can do with them.
Kevin just mentioned that we would probably only want to revert, if at all, if we have something to revert back to.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hahn-kev and @jasonleenaylor)
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 306 at r6 (raw file):
Previously, josephmyers wrote…
Kevin just mentioned that we would probably only want to revert, if at all, if we have something to revert back to.
Thanks for clarifying. Even if the token works, users would be confused if they see 'bearer' instead of their username in the Chorus settings dialog.
We want to give callers the option to revert, e.g. if they know the user/pass is JWT, but default it to saving them.
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @hahn-kev, @jasonleenaylor, and @papeh)
src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 306 at r6 (raw file):
Previously, papeh wrote…
Thanks for clarifying. Even if the token works, users would be confused if they see 'bearer' instead of their username in the Chorus settings dialog.
True! So I'm fine reverting it all the time, to prevent that. I cleaned it up. Let me know what you think
There was a problem hiding this comment.
Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status:
This is necessary so that flexbridge can supply required cloning information and start a repo download without direct/manual user input. This order will come from Fieldworks when launched from CMD or protocol handler with the proper arguments.
Original issue here.
This change is