Add option to ask user identity on every commit (#1251)

* Add option to ask user identity on every commit

* Override author

* Set author in config when not set even with prompt on every commit

* Fix test

* Apply suggestions from code review

Co-authored-by: Frédéric Collonval <[email protected]>

* Cleanup the code

* Fix formatting

* Improve author form layout

* Fix linter

---------

Co-authored-by: Frédéric Collonval <[email protected]>
Co-authored-by: Frédéric Collonval <[email protected]>

README.md

@@ -103,6 +103,7 @@ Once installed, extension behavior can be modified via the following settings wh
 - **displayStatus**: display Git extension status updates in the JupyterLab status bar. If `true`, the extension displays status updates in the JupyterLab status bar, such as when pulling and pushing changes, switching branches, and polling for changes. Depending on the level of extension activity, some users may find the status updates distracting. In which case, setting this to `false` should reduce visual noise.
 - **doubleClickDiff**: double click a file in the Git extension panel to open a diff of the file instead of opening the file for editing.
 - **historyCount**: number of commits shown in the history log, beginning with the most recent. Displaying a larger number of commits can lead to performance degradation, so use caution when modifying this setting.
+- **promptUserIdentity**: Whether to prompt for user name and email on every commit.
 - **refreshIfHidden**: whether to refresh even if the Git tab is hidden; default to `false` (i.e. refresh is turned off if the Git tab is hidden).
 - **refreshInterval**: number of milliseconds between polling the file system for changes. In order to ensure that the UI correctly displays the current repository status, the extension must poll the file system for changes. Longer polling times increase the likelihood that the UI does not reflect the current status; however, longer polling times also incur less performance overhead.
 - **simpleStaging**: enable a simplified concept of staging. When this setting is `true`, all files with changes are automatically staged. When we develop in JupyterLab, we often only care about what files have changed (in the broadest sense) and don't need to distinguish between "tracked" and "untracked" files. Accordingly, this setting allows us to simplify the visual presentation of changes, which is especially useful for those less acquainted with Git.

jupyterlab_git/git.py

@@ -1156,13 +1156,15 @@ async def merge(self, branch: str, path: str) -> dict:
             return {"code": code, "command": " ".join(cmd), "message": error}
         return {"code": code, "message": output.strip()}
 
-    async def commit(self, commit_msg, amend, path):
+    async def commit(self, commit_msg, amend, path, author=None):
         """
         Execute git commit <filename> command & return the result.
 
         If the amend argument is true, amend the commit instead of creating a new one.
         """
         cmd = ["git", "commit"]
+        if author:
+            cmd.extend(["--author", author])
         if amend:
             cmd.extend(["--amend", "--no-edit"])
         else:

jupyterlab_git/handlers.py

@@ -561,7 +561,10 @@ async def post(self, path: str = ""):
         data = self.get_json_body()
         commit_msg = data["commit_msg"]
         amend = data.get("amend", False)
-        body = await self.git.commit(commit_msg, amend, self.url2localpath(path))
+        author = data.get("author")
+        body = await self.git.commit(
+            commit_msg, amend, self.url2localpath(path), author
+        )
 
         if body["code"] != 0:
             self.set_status(500)

schema/plugin.json

@@ -65,6 +65,12 @@
       "description": "Whether to trigger or not a push for each commit.",
       "default": false
     },
+    "promptUserIdentity": {
+      "type": "boolean",
+      "title": "Prompt user identity on commit",
+      "description": "Whether to prompt for user name and email for each commit.",
+      "default": false
+    },
     "openFilesBehindWarning": {
       "type": "boolean",
       "title": "Open files behind warning",

src/__tests__/test-components/GitPanel.spec.tsx

@@ -40,14 +40,15 @@ const mockedResponses: {
  * @private
  * @returns mock settings
  */
-function MockSettings(commitAndPush = true) {
+function MockSettings(commitAndPush = true, promptUserIdentity = false) {
   return {
     changed: {
       connect: () => true,
       disconnect: () => true
     },
     composite: {
-      commitAndPush
+      commitAndPush,
+      promptUserIdentity
     }
   };
 }
@@ -161,7 +162,9 @@ describe('GitPanel', () => {
       expect(configSpy).toHaveBeenCalledTimes(1);
       expect(commitSpy).toHaveBeenCalledTimes(1);
       expect(commitSpy).toHaveBeenCalledWith(
-        commitSummary + '\n\n' + commitDescription + '\n'
+        commitSummary + '\n\n' + commitDescription + '\n',
+        false,
+        null
       );
 
       // Only erase commit message upon success
@@ -175,17 +178,38 @@ describe('GitPanel', () => {
       expect(commitSpy).not.toHaveBeenCalled();
     });
 
+    it('should prompt for user identity if explicitly configured', async () => {
+      configSpy.mockResolvedValue({ options: commitUser });
+
+      props.settings = MockSettings(false, true) as any;
+      const panelWrapper = shallow<GitPanel>(<GitPanel {...props} />);
+      panel = panelWrapper.instance();
+
+      mockUtils.showDialog.mockResolvedValue(dialogValue);
+
+      panel.setState({ commitSummary });
+
+      await panel.commitFiles();
+      expect(configSpy).toHaveBeenCalledTimes(1);
+      expect(configSpy.mock.calls[0]).toHaveLength(0);
+
+      const author = `${commitUser['user.name']} <${commitUser['user.email']}>`;
+      expect(commitSpy).toHaveBeenCalledTimes(1);
+      expect(commitSpy).toHaveBeenCalledWith(commitSummary, false, author);
+    });
+
     it('should prompt for user identity if user.name is not set', async () => {
       configSpy.mockImplementation(mockConfigImplementation('user.email'));
       mockUtils.showDialog.mockResolvedValue(dialogValue);
 
       panel.setState({ commitSummary });
+
       await panel.commitFiles();
       expect(configSpy).toHaveBeenCalledTimes(2);
       expect(configSpy.mock.calls[0]).toHaveLength(0);
       expect(configSpy.mock.calls[1]).toEqual([commitUser]);
       expect(commitSpy).toHaveBeenCalledTimes(1);
-      expect(commitSpy).toHaveBeenCalledWith(commitSummary);
+      expect(commitSpy).toHaveBeenCalledWith(commitSummary, false, null);
     });
 
     it('should prompt for user identity if user.email is not set', async () => {
@@ -198,7 +222,7 @@ describe('GitPanel', () => {
       expect(configSpy.mock.calls[0]).toHaveLength(0);
       expect(configSpy.mock.calls[1]).toEqual([commitUser]);
       expect(commitSpy).toHaveBeenCalledTimes(1);
-      expect(commitSpy).toHaveBeenCalledWith(commitSummary);
+      expect(commitSpy).toHaveBeenCalledWith(commitSummary, false, null);
     });
 
     it('should not commit if no user identity is set and the user rejects the dialog', async () => {

src/components/GitPanel.tsx

@@ -770,17 +770,17 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
     };
 
     try {
-      await this._hasIdentity(this.props.model.pathRepository);
+      const author = await this._hasIdentity(this.props.model.pathRepository);
 
       this.props.logger.log({
         level: Level.RUNNING,
         message: this.props.trans.__('Committing changes...')
       });
 
       if (this.state.commitAmend) {
-        await this.props.model.commit(null, true);
+        await this.props.model.commit(null, true, author);
       } else {
-        await this.props.model.commit(message);
+        await this.props.model.commit(message, false, author);
       }
 
       this.props.logger.log({
@@ -807,19 +807,38 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
    *
    * @param path - repository path
    */
-  private async _hasIdentity(path: string): Promise<void> {
-    // If the repository path changes, check the user identity
-    if (path !== this._previousRepoPath) {
+  private async _hasIdentity(path: string): Promise<string | null> {
+    // If the repository path changes or explicitly configured, check the user identity
+    if (
+      path !== this._previousRepoPath ||
+      this.props.settings.composite['promptUserIdentity']
+    ) {
       try {
-        const data: JSONObject = (await this.props.model.config()) as any;
-        const options: JSONObject = data['options'] as JSONObject;
-        const keys = Object.keys(options);
+        let userOrEmailNotSet = false;
+        let author: Git.IIdentity;
+        let authorOverride: string | null = null;
+
+        if (this.props.model.lastAuthor === null) {
+          const data: JSONObject = (await this.props.model.config()) as any;
+          const options: JSONObject = data['options'] as JSONObject;
+
+          author = {
+            name: (options['user.name'] as string) || '',
+            email: (options['user.email'] as string) || ''
+          };
+          userOrEmailNotSet = !author.name || !author.email;
+        } else {
+          author = this.props.model.lastAuthor;
+        }
 
-        // If the user name or e-mail is unknown, ask the user to set it
-        if (keys.indexOf('user.name') < 0 || keys.indexOf('user.email') < 0) {
+        // If explicitly configured or the user name or e-mail is unknown, ask the user to set it
+        if (
+          this.props.settings.composite['promptUserIdentity'] ||
+          userOrEmailNotSet
+        ) {
           const result = await showDialog({
             title: this.props.trans.__('Who is committing?'),
-            body: new GitAuthorForm()
+            body: new GitAuthorForm({ author, trans: this.props.trans })
           });
 
           if (!result.button.accept) {
@@ -828,13 +847,21 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
             );
           }
 
-          const { name, email } = result.value;
-          await this.props.model.config({
-            'user.name': name,
-            'user.email': email
-          });
+          author = result.value;
+          if (userOrEmailNotSet) {
+            await this.props.model.config({
+              'user.name': author.name,
+              'user.email': author.email
+            });
+          }
+          this.props.model.lastAuthor = author;
+
+          if (this.props.settings.composite['promptUserIdentity']) {
+            authorOverride = `${author.name} <${author.email}>`;
+          }
         }
         this._previousRepoPath = path;
+        return authorOverride;
       } catch (error) {
         if (error instanceof Git.GitResponseError) {
           throw error;

src/model.ts

@@ -198,6 +198,18 @@ export class GitExtension implements IGitExtension {
     }
   }
 
+  /**
+   * Last author
+   *
+   */
+  get lastAuthor(): Git.IIdentity | null {
+    return this._lastAuthor;
+  }
+
+  set lastAuthor(lastAuthor: Git.IIdentity) {
+    this._lastAuthor = lastAuthor;
+  }
+
   /**
    * Git repository status
    */
@@ -670,18 +682,24 @@ export class GitExtension implements IGitExtension {
    *
    * @param message - commit message
    * @param amend - whether this is an amend commit
+   * @param author - override the commit author specified in config
    * @returns promise which resolves upon committing file changes
    *
    * @throws {Git.NotInRepository} If the current path is not a Git repository
    * @throws {Git.GitResponseError} If the server response is not ok
    * @throws {ServerConnection.NetworkError} If the request cannot be made
    */
-  async commit(message?: string, amend = false): Promise<void> {
+  async commit(
+    message?: string,
+    amend = false,
+    author?: string
+  ): Promise<void> {
     const path = await this._getPathRepository();
     await this._taskHandler.execute('git:commit:create', async () => {
       await requestAPI(URLExt.join(path, 'commit'), 'POST', {
         commit_msg: message,
-        amend: amend
+        amend: amend,
+        author: author
       });
     });
     await this.refresh();
@@ -2118,6 +2136,7 @@ export class GitExtension implements IGitExtension {
   private _selectedHistoryFile: Git.IStatusFile | null = null;
   private _hasDirtyFiles = false;
   private _credentialsRequired = false;
+  private _lastAuthor: Git.IIdentity | null = null;
 
   // Configurable
   private _statusForDirtyState: Git.Status[] = ['staged', 'partially-staged'];

src/widgets/AuthorBox.ts

@@ -1,4 +1,5 @@
 import { Dialog } from '@jupyterlab/apputils';
+import { TranslationBundle } from '@jupyterlab/translation';
 import { Widget } from '@lumino/widgets';
 import { Git } from '../tokens';
 
@@ -9,26 +10,37 @@ export class GitAuthorForm
   extends Widget
   implements Dialog.IBodyWidget<Git.IIdentity>
 {
-  constructor() {
+  constructor({
+    author,
+    trans
+  }: {
+    author: Git.IIdentity;
+    trans: TranslationBundle;
+  }) {
     super();
-    this.node.appendChild(this.createBody());
+    this._populateForm(author, trans);
   }
 
-  private createBody(): HTMLElement {
-    const node = document.createElement('div');
-    const text = document.createElement('span');
-    this._name = document.createElement('input');
-    this._email = document.createElement('input');
+  private _populateForm(
+    author: Git.IIdentity,
+    trans?: TranslationBundle
+  ): void {
+    const nameLabel = document.createElement('label');
+    nameLabel.textContent = trans.__('Committer name:');
+    const emailLabel = document.createElement('label');
+    emailLabel.textContent = trans.__('Committer email:');
 
-    node.className = 'jp-RedirectForm';
-    text.textContent = 'Enter your name and email for commit';
+    this._name = nameLabel.appendChild(document.createElement('input'));
+    this._email = emailLabel.appendChild(document.createElement('input'));
     this._name.placeholder = 'Name';
+    this._email.type = 'text';
     this._email.placeholder = 'Email';
+    this._email.type = 'email';
+    this._name.value = author.name;
+    this._email.value = author.email;
 
-    node.appendChild(text);
-    node.appendChild(this._name);
-    node.appendChild(this._email);
-    return node;
+    this.node.appendChild(nameLabel);
+    this.node.appendChild(emailLabel);
   }
 
   /**