Add GitCommitterIdentityHandler and change identity check

metalhead95 · metalhead95 · commit 5ce262457e37 · 2020-07-07T17:41:04.000+05:30
User should be asked for name and email only
when none of the following conditions are satisfied -
* git config contains user.name and user.email
* GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL are set
* EMAIL is set

If any of the above is true, git doesn't show warning
to set the author info.
diff --git a/jupyterlab_git/handlers.py b/jupyterlab_git/handlers.py
@@ -514,6 +514,23 @@ async def post(self):
         self.finish(json.dumps(response))
 
 
+class GitCommitterIdentityHandler(GitHandler):
+    """
+    Handler for checking if user identity is set
+    """
+
+    @web.authenticated
+    async def get(self):
+        identity_established = False
+        top_repo_path = self.get_argument("path")
+        response = await self.git.config(top_repo_path)
+        if "user.name" in response["options"] and "user.email" in response["options"]:
+            identity_established = True
+        if "GIT_COMMITTER_NAME" in os.environ and "GIT_COMMITTER_EMAIL" in os.environ or "EMAIL" in os.environ:
+            identity_established = True
+        self.finish(json.dumps({"identityEstablished": identity_established}))
+
+
 class GitDiffContentHandler(GitHandler):
     """
     Handler for plain text diffs. Uses git show $REF:$FILE
@@ -576,6 +593,7 @@ def setup_handlers(web_app):
         ("/git/show_top_level", GitShowTopLevelHandler),
         ("/git/status", GitStatusHandler),
         ("/git/upstream", GitUpstreamHandler),
+        ("/git/committer_identity", GitCommitterIdentityHandler),
     ]
 
     # add the baseurl to our paths
diff --git a/jupyterlab_git/tests/test_handlers.py b/jupyterlab_git/tests/test_handlers.py
@@ -11,6 +11,7 @@
     GitLogHandler,
     GitPushHandler,
     GitUpstreamHandler,
+    GitCommitterIdentityHandler,
     setup_handlers,
 )
 
@@ -62,6 +63,71 @@ def test_all_history_handler_localbranch(self, mock_git):
         }
 
 
+class TestCommitterIdentityHandler(ServerTest):
+    @patch("jupyterlab_git.handlers.GitCommitterIdentityHandler.git", spec=Git)
+    def test_name_and_email_from_config(self, mock_git):
+        config_options = {"code": 0, "options": {"user.name": "foo", "user.email": "bar"}}
+        mock_git.config.return_value = maybe_future(config_options)
+
+        response = self.tester.get(["/committer_identity?path=/path/to/repo"])
+        payload = response.json()
+        assert payload == {"identityEstablished": True}
+
+    @patch("jupyterlab_git.handlers.GitCommitterIdentityHandler.git", spec=Git)
+    def test_name_missing_from_config(self, mock_git):
+        config_options = {"code": 0, "options": {"user.email": "bar"}}
+        mock_git.config.return_value = maybe_future(config_options)
+
+        response = self.tester.get(["/committer_identity?path=/path/to/repo"])
+        payload = response.json()
+        assert payload == {"identityEstablished": False}
+
+    @patch("jupyterlab_git.handlers.GitCommitterIdentityHandler.git", spec=Git)
+    def test_email_missing_from_config(self, mock_git):
+        config_options = {"code": 0, "options": {"user.name": "bar"}}
+        mock_git.config.return_value = maybe_future(config_options)
+
+        response = self.tester.get(["/committer_identity?path=/path/to/repo"])
+        payload = response.json()
+        assert payload == {"identityEstablished": False}
+
+    @patch("jupyterlab_git.handlers.GitCommitterIdentityHandler.git", spec=Git)
+    @patch.dict(os.environ, {"GIT_COMMITTER_NAME": "foo", "GIT_COMMITTER_EMAIL": "bar@xyz.com"})
+    def test_committer_name_email_var(self, mock_git):
+        mock_git.config.return_value = maybe_future({"code": 0, "options": {}})
+
+        response = self.tester.get(["/committer_identity?path=/path/to/repo"])
+        payload = response.json()
+        assert payload == {"identityEstablished": True}
+
+    @patch("jupyterlab_git.handlers.GitCommitterIdentityHandler.git", spec=Git)
+    @patch.dict(os.environ, {"GIT_COMMITTER_NAME": "foo"})
+    def test_missing_committer_email_var(self, mock_git):
+        mock_git.config.return_value = maybe_future({"code": 0, "options": {}})
+
+        response = self.tester.get(["/committer_identity?path=/path/to/repo"])
+        payload = response.json()
+        assert payload == {"identityEstablished": False}
+
+    @patch("jupyterlab_git.handlers.GitCommitterIdentityHandler.git", spec=Git)
+    @patch.dict(os.environ, {"GIT_COMMITTER_EMAIL": "bar@xyz.com"})
+    def test_committer_name_var(self, mock_git):
+        mock_git.config.return_value = maybe_future({"code": 0, "options": {}})
+
+        response = self.tester.get(["/committer_identity?path=/path/to/repo"])
+        payload = response.json()
+        assert payload == {"identityEstablished": False}
+
+    @patch("jupyterlab_git.handlers.GitCommitterIdentityHandler.git", spec=Git)
+    @patch.dict(os.environ, {"EMAIL": "foo@bar.com"})
+    def test_committer_name_email_var(self, mock_git):
+        mock_git.config.return_value = maybe_future({"code": 0, "options": {}})
+
+        response = self.tester.get(["/committer_identity?path=/path/to/repo"])
+        payload = response.json()
+        assert payload == {"identityEstablished": True}
+
+
 class TestBranch(ServerTest):
     @patch("jupyterlab_git.handlers.GitBranchHandler.git", spec=Git)
     def test_branch_handler_localbranch(self, mock_git):
diff --git a/src/components/GitPanel.tsx b/src/components/GitPanel.tsx
@@ -3,7 +3,6 @@ import { PathExt } from '@jupyterlab/coreutils';
 import { FileBrowserModel } from '@jupyterlab/filebrowser';
 import { IRenderMimeRegistry } from '@jupyterlab/rendermime';
 import { ISettingRegistry } from '@jupyterlab/settingregistry';
-import { JSONObject } from '@lumino/coreutils';
 import Tab from '@material-ui/core/Tab';
 import Tabs from '@material-ui/core/Tabs';
 import * as React from 'react';
@@ -383,13 +382,13 @@ export class GitPanel extends React.Component<
     // If the repository path changes, check the user identity
     if (path !== this._previousRepoPath) {
       try {
-        let res = await this.props.model.config();
+        let res = await this.props.model.isCommitterIdentitySet();
         if (res.ok) {
-          const options: JSONObject = (await res.json()).options;
-          const keys = Object.keys(options);
+          const identityEstablished: boolean = (await res.json())
+            .identityEstablished;
 
           // 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 (!identityEstablished) {
             const result = await showDialog({
               title: 'Who is committing?',
               body: new GitAuthorForm()
diff --git a/src/model.ts b/src/model.ts
@@ -571,6 +571,39 @@ export class GitExtension implements IGitExtension {
     }
   }
 
+  async isCommitterIdentitySet(): Promise<Response> {
+    await this.ready;
+    const path = this.pathRepository;
+
+    if (path === null) {
+      return Promise.resolve(
+        new Response(
+          JSON.stringify({
+            code: -1,
+            message: 'Not in a git repository.'
+          })
+        )
+      );
+    }
+
+    try {
+      const response = await httpGitRequest(
+        `/git/committer_identity?path=${path}`,
+        'GET',
+        null
+      );
+
+      if (!response.ok) {
+        const jsonData = await response.json();
+        throw new ServerConnection.ResponseError(response, jsonData.message);
+      }
+
+      return response;
+    } catch (err) {
+      throw new ServerConnection.NetworkError(err);
+    }
+  }
+
   /**
    * Make request to revert changes from selected commit
    *
diff --git a/tests/test-components/GitPanel.spec.tsx b/tests/test-components/GitPanel.spec.tsx
@@ -108,14 +108,11 @@ describe('GitPanel', () => {
 
       // Mock identity look up
       const identity = jest
-        .spyOn(GitModel.prototype, 'config')
+        .spyOn(GitModel.prototype, 'isCommitterIdentitySet')
         .mockResolvedValue(
           new Response(
             JSON.stringify({
-              options: {
-                'user.name': 'John Snow',
-                'user.email': 'john.snow@winteris.com'
-              }
+              identityEstablished: true
             }),
             { status: 201 }
           )
@@ -136,81 +133,29 @@ describe('GitPanel', () => {
       spy.mockRestore();
     });
 
-    it('should prompt for user identity if user.name is not set', async () => {
+    it('should prompt for user identity if not set', async () => {
       const spy = jest.spyOn(GitModel.prototype, 'commit');
 
-      // Mock identity look up
-      const identity = jest
+      const config = jest
         .spyOn(GitModel.prototype, 'config')
         .mockImplementation(options => {
           let response: Response = null;
-          if (options === undefined) {
-            response = new Response(
-              JSON.stringify({
-                options: {
-                  'user.email': 'john.snow@winteris.com'
-                }
-              }),
-              { status: 201 }
-            );
-          } else {
-            response = new Response('', { status: 201 });
-          }
+          response = new Response('', { status: 201 });
           return Promise.resolve(response);
         });
-      const mock = apputils as jest.Mocked<typeof apputils>;
-      mock.showDialog.mockResolvedValue({
-        button: {
-          accept: true,
-          caption: '',
-          className: '',
-          displayType: 'default',
-          iconClass: '',
-          iconLabel: '',
-          label: ''
-        },
-        value: {
-          name: 'John Snow',
-          email: 'john.snow@winteris.com'
-        }
-      });
-
-      const panel = new GitPanel(props);
-      await panel.commitStagedFiles('Initial commit');
-      expect(identity).toHaveBeenCalledTimes(2);
-      expect(identity.mock.calls[0]).toHaveLength(0);
-      expect(identity.mock.calls[1]).toEqual([
-        {
-          'user.name': 'John Snow',
-          'user.email': 'john.snow@winteris.com'
-        }
-      ]);
-      expect(spy).toHaveBeenCalledTimes(1);
-      expect(spy).toHaveBeenCalledWith('Initial commit');
-    });
-
-    it('should prompt for user identity if user.email is not set', async () => {
-      const spy = jest.spyOn(GitModel.prototype, 'commit');
 
       // Mock identity look up
       const identity = jest
-        .spyOn(GitModel.prototype, 'config')
-        .mockImplementation(options => {
-          let response: Response = null;
-          if (options === undefined) {
-            response = new Response(
-              JSON.stringify({
-                options: {
-                  'user.name': 'John Snow'
-                }
-              }),
-              { status: 201 }
-            );
-          } else {
-            response = new Response('', { status: 201 });
-          }
-          return Promise.resolve(response);
-        });
+        .spyOn(GitModel.prototype, 'isCommitterIdentitySet')
+        .mockResolvedValue(
+          new Response(
+            JSON.stringify({
+              identityEstablished: false
+            }),
+            { status: 201 }
+          )
+        );
+
       const mock = apputils as jest.Mocked<typeof apputils>;
       mock.showDialog.mockResolvedValue({
         button: {
@@ -230,9 +175,9 @@ describe('GitPanel', () => {
 
       const panel = new GitPanel(props);
       await panel.commitStagedFiles('Initial commit');
-      expect(identity).toHaveBeenCalledTimes(2);
-      expect(identity.mock.calls[0]).toHaveLength(0);
-      expect(identity.mock.calls[1]).toEqual([
+      expect(identity).toHaveBeenCalledTimes(1);
+      expect(config).toHaveBeenCalledTimes(1);
+      expect(config.mock.calls[0]).toEqual([
         {
           'user.name': 'John Snow',
           'user.email': 'john.snow@winteris.com'
@@ -247,21 +192,16 @@ describe('GitPanel', () => {
 
       // Mock identity look up
       const identity = jest
-        .spyOn(GitModel.prototype, 'config')
-        .mockImplementation(options => {
-          let response: Response = null;
-          if (options === undefined) {
-            response = new Response(
-              JSON.stringify({
-                options: {}
-              }),
-              { status: 201 }
-            );
-          } else {
-            response = new Response('', { status: 201 });
-          }
-          return Promise.resolve(response);
-        });
+        .spyOn(GitModel.prototype, 'isCommitterIdentitySet')
+        .mockResolvedValue(
+          new Response(
+            JSON.stringify({
+              identityEstablished: false
+            }),
+            { status: 201 }
+          )
+        );
+
       const mock = apputils as jest.Mocked<typeof apputils>;
       mock.showDialog.mockResolvedValue({
         button: {
@@ -279,7 +219,6 @@ describe('GitPanel', () => {
       const panel = new GitPanel(props);
       await panel.commitStagedFiles('Initial commit');
       expect(identity).toHaveBeenCalledTimes(1);
-      expect(identity).toHaveBeenCalledWith();
       expect(spy).not.toHaveBeenCalled();
     });
   });
