bug: modified password placeholder text depending if remote URI is github (Fix #1176) #1220

shawnesquivel · 2023-02-08T21:41:07Z

This fixes #1176.

Description of Changes

Check to see if the remote URI matches the GitHub expression
Change the password placeholder text accordingly

Git Push

Git.Push.mov

Git Clone

Git.Clone.mov

Git Pull

Git.Pull.mov

Collaborated with @basokant on this!

welcome · 2023-02-08T21:41:09Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

github-actions · 2023-02-08T21:41:20Z

👈 Launch a binder notebook on branch shawnesquivel/jupyterlab-git/issue-1176

fcollonval

Thanks @shawnesquivel

As mentioned in the issue, the trouble here is that this is specific to a Git server policy. For example other providers such GitLab or BitBucket may still allow that authentication system.

This means we need to condition the placeholder with the remote repository URI.

fcollonval · 2023-02-09T11:19:26Z

The dialog is spinned there:

jupyterlab-git/src/commandsAndMenu.tsx

Line 1626 in 1125869

body: new GitCredentialsForm(

So we could pass the placeholder to the dialog constructor.

Now about the remote repository URI, this is gonna be more complex. In the case of a clone operation, we have the url in args. But for pull and push, we know the remote name but not the URI. You could find the URI by requesting the remotes with the following function:

jupyterlab-git/src/model.ts

Line 460 in 1125869

async getRemotes(): Promise<Git.IGitRemote[]> {

Does that make sense?

shawnesquivel · 2023-02-14T02:10:45Z

Hi @fcollonval!

Added some changes + working screenshot.

It is working for GitHub but remains as the default 'password / personal access token' otherwise.

@basokant also worked with me again on this one, he was a big help!

Let me know if there are any other changes to be made.

fcollonval

Thanks @shawnesquivel

This is looking good. We now have to improve the url search depending on the operation to be carried out.

fcollonval · 2023-02-15T12:33:14Z

src/widgets/CredentialsBox.tsx

@@ -10,13 +10,16 @@ export class GitCredentialsForm
  extends Widget
  implements Dialog.IBodyWidget<Git.IAuth>
 {
+  passwordPlaceholder: string;


Purely styling comment:

Could you place this attribute at the bottom of the class with the other attributes and make it private (with the convention of starting with _)?

Done. I assume _ is only for the private property, so I didn't change it on the parameter.

fcollonval · 2023-02-15T12:34:27Z

src/commandsAndMenu.tsx

+      const result = remoteURI[0]?.url.match(re);
+      const gitProvider = result[1];


Result may be undefined so you should protect the code against that; something like:

Suggested change

const result = remoteURI[0]?.url.match(re);

const gitProvider = result[1];

const result = remoteURI[0]?.url.match(re) ?? [];

const gitProvider = result[1];

fcollonval · 2023-02-15T12:38:02Z

src/commandsAndMenu.tsx

+      const remoteURI = await model.getRemotes().then(remote => {
+        return remote;
+      });


Good this is the idea indeed. But now we need to be smart depending on the operation type. If the operation is Operation.Clone, we don't need to request the remotes as we can look directly at args.url. In the cases of push and pull, we should not take remoteURI[0] but we should find the item matching args.remote. Then for the fetch case, let's use your fallback of only testing remoteURI[0].

@fcollonval

For Pull:
Pull - I tried accessing args. When logging it, I noticed args is an empty object (no remote property), so I passed it in using the code below, which showed me that args.remote = 'origin'. But I don't think I fully understand what you mean by 'finding the item matching args.remote', do you mind explaining further? I guess I just don't see how the args.remote is going to tell us that the git provider is github (if that was what you were suggesting).

commandsAndMenu.tsx Line 383

/** Add git pull command */ commands.addCommand(CommandIDs.gitPull, { execute: async args => { try { // copy how commandIDS.gitPush grabs remote let remote; const result = await showDialog({ title: trans.__('Please select push options.'), body: new AdvancedPushForm(trans, gitModel), buttons: [ Dialog.cancelButton({ label: trans.__('Cancel') }), Dialog.okButton({ label: trans.__('Proceed') }) ] }); remote = result.value.remoteName; // added args as an additional param const details = await showGitOperationDialog( gitModel, Operation.Pull, trans, (args = { remote }) );

For Push

By default, args is an object with remote property, but it was undefined. So, maybe args.remote is not defined if we're not authenticated?

git uses name for remote to be more user friendly. Then it uses a db to get the URI from the name.

You can see that by executing in a terminal git remote -vv; the output will look like:

origin git@github.com:fcollonval/jupyterlab.git (fetch) origin git@github.com:fcollonval/jupyterlab.git (push) trungleduc https://github.com/trungleduc/jupyterlab (fetch) trungleduc https://github.com/trungleduc/jupyterlab (push) upstream https://github.com/jupyterlab/jupyterlab.git (fetch) upstream https://github.com/jupyterlab/jupyterlab.git (push)

Name origin refers git@github.com:fcollonval/jupyterlab.git in the above example.

As you discover (:clap: for that), you could get the name passing the remote variable. But to determine the associated URI, you first need to call getRemote (as you have done already). It returns you an array of Git.IGitRemote. That interface has the following definition:

interface IGitRemote { url: string; name: string; }

So now that you know remote you need to find the IGitRemote object in the array with that name. Then you will be able to test the URL.

Does it clarify what you need to do?

Regarding push, if it is undefined, you could still apply the same logic and fallback to remoteURI[0].url if it is undefined.

We could improve this by having the backend tell use which of the remote is the default. But let's try not to increase the scope of this PR too much.

Hi @fcollonval !

Yes this was a very thorough explanation, thank you!

I was able to add the edits you requested for pull, push, clone, and fetch.

I have some more questions:

How do I test git fetch?

I tried using git fetch -all which returned 'Fetching origin'

However, it didn't throw any errors even though we are not authenticated, so I'm not sure how to proceed.

In git pull, I passed the args to the git-pull catch block by using the following code. It's wrong since it creates an unnecessary dialog box just to get the current remote name. Is there some other way of getting the current remote name? If not, I'll just remove this section as there are fallbacks in place already.

let remote; const result = await showDialog({ title: trans.__('Please select push options.'), body: new AdvancedPushForm(trans, gitModel), buttons: [ Dialog.cancelButton({ label: trans.__('Cancel') }), Dialog.okButton({ label: trans.__('Proceed') }) ] }); remote = result.value.remoteName;

I grouped git pull, push and force in the catch block because that made sense to me, but there may be some nuances here that I'm overlooking.

fcollonval

Thanks @shawnesquivel

Here are some comments and suggestions

fcollonval · 2023-02-22T14:16:41Z

src/commandsAndMenu.tsx

+        // copy how gitPush grabs remote - this needs to be fixed as it creates a Git Push UI
+        // is there a better way to grab the remoteName?
+        const result = await showDialog({
+          title: trans.__('Please select push options.'),
+          body: new AdvancedPushForm(trans, gitModel),
+          buttons: [
+            Dialog.cancelButton({ label: trans.__('Cancel') }),
+            Dialog.okButton({ label: trans.__('Proceed') })
+          ]
+        });
+        const remote = result.value.remoteName;
+


Indeed this is not great.

As pulling concerned the current branch, you could introspect gitModel.currentBranch.upstream. If it exists it should start with the remote name; as seen in the example:

jupyterlab-git/tests/test-components/BranchMenu.spec.tsx

Line 51 in 35f712f

upstream: 'origin/patch-007',

src/commandsAndMenu.tsx

fcollonval · 2023-02-22T14:21:31Z

src/commandsAndMenu.tsx

      // If the error is an authentication error, ask the user credentials
      const credentials = await showDialog({
        title: trans.__('Git credentials required'),
        body: new GitCredentialsForm(
          trans,
          trans.__('Enter credentials for remote repository'),
-          retry ? trans.__('Incorrect username or password.') : ''
+          retry ? trans.__('Incorrect username or password.') : '',
+          trans.__(gitPasswordPlaceholder)


This unfortunately will not work as expected. To figure out all the string that requires translation, we cannot rely on executing the code as it may be very complex to get the application in all possible state. So the strings are extracted from a static analysis of the code; i.e. all string passed to trans.__ (and similar) are extracted in the to be localized string database. This means that you need to wrap the constant strings you are setting gitPasswordPlaceholder with (i.e. lines 1667 and 1714).

Suggested change

trans.__(gitPasswordPlaceholder)

gitPasswordPlaceholder

fcollonval · 2023-02-22T14:21:57Z

src/widgets/CredentialsBox.tsx

  constructor(
    trans: TranslationBundle,
    textContent = trans.__('Enter credentials for remote repository'),
-    warningContent = ''
+    warningContent = '',
+    passwordPlaceholder = 'password / personal access token'


For the reason explain earlier.

Suggested change

passwordPlaceholder = 'password / personal access token'

passwordPlaceholder = trans.__('password / personal access token')

fcollonval · 2023-02-22T14:22:17Z

src/widgets/CredentialsBox.tsx

-    this._password.placeholder = this._trans.__(
-      'password / personal access token'
-    );
+    this._password.placeholder = this._trans.__(this._passwordPlaceholder);


Suggested change

this._password.placeholder = this._trans.__(this._passwordPlaceholder);

this._password.placeholder = this._passwordPlaceholder;

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

…lab-git into issue-1176 merged changes

shawnesquivel · 2023-02-23T18:35:58Z

@fcollonval, looks like the tests are passing now! I still haven't tested git fetch yet though as per my previous comment. Everything else seems to be working.

fcollonval

Thanks @shawnesquivel

I added two comments for alternative ways. But the code is good as is. So I'm gonna merge.

fcollonval · 2023-03-03T16:11:59Z

src/commandsAndMenu.tsx

+    if (currentRemote) {
+      return currentRemote?.url;
+    } else {
+      return '';
+    }


FYI you could have use a denser code:

Suggested change

if (currentRemote) {

return currentRemote?.url;

} else {

return '';

}

return currentRemote?.url ?? '';

fcollonval · 2023-03-03T16:14:23Z

src/commandsAndMenu.tsx

+          break;
+      }
+      // GitHub only verifies with personal access tokens
+      if (remoteGitProvider && remoteGitProvider.toLowerCase() === 'github') {


FYI you do not need to test for remoteGitProvider being defined and not empty string. The second test is sufficient.

Suggested change

if (remoteGitProvider && remoteGitProvider.toLowerCase() === 'github') {

if (remoteGitProvider.toLowerCase() === 'github') {

welcome · 2023-03-03T16:15:22Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

modified password placeholder text to only include PAT

2a8eca6

shawnesquivel changed the title ~~bug: modified password placeholder text to only include PAT~~ bug: modified password placeholder text to only include PAT (Fix #1176) Feb 8, 2023

fcollonval requested changes Feb 9, 2023

View reviewed changes

shawnesquivel marked this pull request as draft February 9, 2023 16:18

shawnesquivel marked this pull request as ready for review February 14, 2023 00:59

shawnesquivel changed the title ~~bug: modified password placeholder text to only include PAT (Fix #1176)~~ bug: modified password placeholder text depending if remote URI is github (Fix #1176) Feb 14, 2023

changed placeholder text if remote URI matches github https

a808648

removed redundant type annotation

419087f

fcollonval requested changes Feb 15, 2023

View reviewed changes

shawnesquivel added 7 commits February 18, 2023 20:40

get remote for no-auth git pull

680249a

cleaned code and added catch for git clone

b1cd6e0

removed comment

1fe9000

removed old code

9a3fee2

fixed no-case-declarations error in the catch switch block

596d7c7

fixed and ignored no-case-declaration errors

0cfd591

removed redundant type annotation

61591fe

fcollonval reviewed Feb 22, 2023

View reviewed changes

shawnesquivel and others added 4 commits February 22, 2023 10:14

Update src/commandsAndMenu.tsx

1df96fe

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

updated translation strings and simplified gitPull

166b9ac

Merge branch 'issue-1176' of https://github.com/shawnesquivel/jupyter…

84e16c9

…lab-git into issue-1176 merged changes

make variable names more descriptive

7efe5d8

fcollonval approved these changes Mar 3, 2023

View reviewed changes

fcollonval merged commit 1ecee9d into jupyterlab:master Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug: modified password placeholder text depending if remote URI is github (Fix #1176) #1220

bug: modified password placeholder text depending if remote URI is github (Fix #1176) #1220

shawnesquivel commented Feb 8, 2023 •

edited

Loading

welcome bot commented Feb 8, 2023

github-actions bot commented Feb 8, 2023

fcollonval left a comment •

edited

Loading

fcollonval commented Feb 9, 2023 •

edited

Loading

shawnesquivel commented Feb 14, 2023

fcollonval left a comment

fcollonval Feb 15, 2023

shawnesquivel Feb 15, 2023 •

edited

Loading

fcollonval Feb 15, 2023

fcollonval Feb 15, 2023

shawnesquivel Feb 17, 2023 •

edited

Loading

fcollonval Feb 17, 2023

shawnesquivel Feb 20, 2023 •

edited

Loading

fcollonval left a comment

fcollonval Feb 22, 2023

fcollonval Feb 22, 2023

fcollonval Feb 22, 2023

fcollonval Feb 22, 2023

shawnesquivel commented Feb 23, 2023 •

edited

Loading

fcollonval left a comment

fcollonval Mar 3, 2023

fcollonval Mar 3, 2023

welcome bot commented Mar 3, 2023

		const result = remoteURI[0]?.url.match(re);
		const gitProvider = result[1];

	passwordPlaceholder = 'password / personal access token'
	passwordPlaceholder = trans.__('password / personal access token')

	this._password.placeholder = this._trans.__(this._passwordPlaceholder);
	this._password.placeholder = this._passwordPlaceholder;

	if (remoteGitProvider && remoteGitProvider.toLowerCase() === 'github') {
	if (remoteGitProvider.toLowerCase() === 'github') {

bug: modified password placeholder text depending if remote URI is github (Fix #1176) #1220

bug: modified password placeholder text depending if remote URI is github (Fix #1176) #1220

Conversation

shawnesquivel commented Feb 8, 2023 • edited Loading

welcome bot commented Feb 8, 2023

github-actions bot commented Feb 8, 2023

fcollonval left a comment • edited Loading

Choose a reason for hiding this comment

fcollonval commented Feb 9, 2023 • edited Loading

shawnesquivel commented Feb 14, 2023

fcollonval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnesquivel Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnesquivel Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnesquivel Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

fcollonval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnesquivel commented Feb 23, 2023 • edited Loading

fcollonval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

welcome bot commented Mar 3, 2023

shawnesquivel commented Feb 8, 2023 •

edited

Loading

fcollonval left a comment •

edited

Loading

fcollonval commented Feb 9, 2023 •

edited

Loading

shawnesquivel Feb 15, 2023 •

edited

Loading

shawnesquivel Feb 17, 2023 •

edited

Loading

shawnesquivel Feb 20, 2023 •

edited

Loading

shawnesquivel commented Feb 23, 2023 •

edited

Loading