sillsdev / chorus

End-user collaboration library via 3-way xml merging and hg dvcs under the hood
6 stars 26 forks source link

Add public StartClone() method #325

Closed josephmyers closed 10 months ago

josephmyers commented 11 months ago

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 Reviewable

github-actions[bot] commented 11 months ago

Test Results

       2 files  ±0     202 suites  ±0   43m 59s :stopwatch: -48s    875 tests ±0     853 :heavy_check_mark: ±0  22 :zzz: ±0  0 :x: ±0  1 996 runs  ±0  1 931 :heavy_check_mark: ±0  65 :zzz: ±0  0 :x: ±0 

Results for commit 473c7987. ± Comparison against base commit 84e6de03.

:recycle: This comment has been updated with latest results.

ermshiperete commented 11 months ago

This change deserves a line in CHANGELOG.md.

papeh commented 11 months ago

I'm a bit unclear. The commit comment on https://github.com/sillsdev/flexbridge/pull/388 says that a dialog will be shown to confirm the download, and GetCloneFromInternetDialog seems to be the only dialog shown, but in this PR, it looks like StartClone() bypasses all user interaction.

Will there be user interaction? (I assume you wouldn't want the user to edit anything, except perhaps the local project name.)

hahn-kev commented 11 months ago

@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.

josephmyers commented 11 months ago

@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 😄)

josephmyers commented 11 months ago

@papeh I did your first two comments and will wait for more guidance on the second two. Let me know what you think. Thanks

josephmyers commented 11 months ago

Looking good to me. Always a fan of making things simpler!

josephmyers commented 10 months ago

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?

@hahn-kev

hahn-kev commented 10 months ago

src/Chorus/UI/Clone/GetCloneFromInternetDialog.cs line 306 at r6 (raw file):

Previously, josephmyers wrote…
@hahn-kev

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.