Fail when creating a draft results in HTTP 200

Bug: Issue 7763
Change-Id: Ib16a486efdd4e5bbf4c2f1c8f8d5cd180980e1c5

polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js

@@ -40,6 +40,11 @@
     SEND_DIFF_DRAFT: 'sendDiffDraft',
   };
 
+  const CREATE_DRAFT_UNEXPECTED_STATUS_MESSAGE =
+      'Saving draft resulted in HTTP 200 (OK) but expected HTTP 201 (Created)';
+  const HEADER_REPORTING_BLACKLIST = /^set-cookie$/i;
+
+
   Polymer({
     is: 'gr-rest-api-interface',
 
@@ -1636,6 +1641,7 @@
     },
 
     _sendDiffDraftRequest(method, changeNum, patchNum, draft) {
+      const isCreate = !draft.id && method === 'PUT';
       let endpoint = '/drafts';
       if (draft.id) {
         endpoint += '/' + draft.id;
@@ -1652,6 +1658,11 @@
       const promise = this.getChangeURLAndSend(changeNum, method, patchNum,
           endpoint, body);
       this._pendingRequests[Requests.SEND_DIFF_DRAFT].push(promise);
+
+      if (isCreate) {
+        return this._failForCreate200(promise);
+      }
+
       return promise;
     },
 
@@ -1996,5 +2007,34 @@
           `/files/${encodedPath}/blame`, patchNum, undefined, undefined,
           opt_base ? {base: 't'} : undefined);
     },
+
+    /**
+     * Modify the given create draft request promise so that it fails and throws
+     * an error if the response bears HTTP status 200 instead of HTTP 201.
+     * @see Issue 7763
+     * @param {Promise} promise The original promise.
+     * @return {Promise} The modified promise.
+     */
+    _failForCreate200(promise) {
+      return promise.then(result => {
+        if (result.status === 200) {
+          // Read the response headers into an object representation.
+          const headers = Array.from(result.headers.entries())
+              .reduce((obj, [key, val]) => {
+                if (!HEADER_REPORTING_BLACKLIST.test(key)) {
+                  obj[key] = val;
+                }
+                return obj;
+              }, {});
+          const err = new Error([
+            CREATE_DRAFT_UNEXPECTED_STATUS_MESSAGE,
+            JSON.stringify(headers),
+          ].join('\n'));
+          // Throw the error so that it is caught by gr-reporting.
+          throw err;
+        }
+        return result;
+      });
+    },
   });
 })();

polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html

@@ -651,24 +651,83 @@ limitations under the License.
       });
     });
 
-    test('_sendDiffDraft pending requests tracked', () => {
-      const obj = element._pendingRequests;
-      sandbox.stub(element, 'getChangeURLAndSend', () => mockPromise());
-      assert.notOk(element.hasPendingDiffDrafts());
+    suite('draft comments', () => {
+      test('_sendDiffDraftRequest pending requests tracked', () => {
+        const obj = element._pendingRequests;
+        sandbox.stub(element, 'getChangeURLAndSend', () => mockPromise());
+        assert.notOk(element.hasPendingDiffDrafts());
 
-      element._sendDiffDraftRequest(null, null, null, {});
-      assert.equal(obj.sendDiffDraft.length, 1);
-      assert.isTrue(!!element.hasPendingDiffDrafts());
+        element._sendDiffDraftRequest(null, null, null, {});
+        assert.equal(obj.sendDiffDraft.length, 1);
+        assert.isTrue(!!element.hasPendingDiffDrafts());
 
-      element._sendDiffDraftRequest(null, null, null, {});
-      assert.equal(obj.sendDiffDraft.length, 2);
-      assert.isTrue(!!element.hasPendingDiffDrafts());
+        element._sendDiffDraftRequest(null, null, null, {});
+        assert.equal(obj.sendDiffDraft.length, 2);
+        assert.isTrue(!!element.hasPendingDiffDrafts());
 
-      for (const promise of obj.sendDiffDraft) { promise.resolve(); }
+        for (const promise of obj.sendDiffDraft) { promise.resolve(); }
 
-      return element.awaitPendingDiffDrafts().then(() => {
-        assert.equal(obj.sendDiffDraft.length, 0);
-        assert.isFalse(!!element.hasPendingDiffDrafts());
+        return element.awaitPendingDiffDrafts().then(() => {
+          assert.equal(obj.sendDiffDraft.length, 0);
+          assert.isFalse(!!element.hasPendingDiffDrafts());
+        });
+      });
+
+      suite('_failForCreate200', () => {
+        test('_sendDiffDraftRequest checks for 200 on create', () => {
+          const sendPromise = Promise.resolve();
+          sandbox.stub(element, 'getChangeURLAndSend').returns(sendPromise);
+          const failStub = sandbox.stub(element, '_failForCreate200')
+              .returns(Promise.resolve());
+          return element._sendDiffDraftRequest('PUT', 123, 4, {}).then(() => {
+            assert.isTrue(failStub.calledOnce);
+            assert.isTrue(failStub.calledWithExactly(sendPromise));
+          });
+        });
+
+        test('_sendDiffDraftRequest no checks for 200 on non create', () => {
+          sandbox.stub(element, 'getChangeURLAndSend')
+              .returns(Promise.resolve());
+          const failStub = sandbox.stub(element, '_failForCreate200')
+              .returns(Promise.resolve());
+          return element._sendDiffDraftRequest('PUT', 123, 4, {id: '123'})
+              .then(() => {
+                assert.isFalse(failStub.called);
+              });
+        });
+
+        test('_failForCreate200 fails on 200', done => {
+          const result = {
+            ok: true,
+            status: 200,
+            headers: {entries: () => [
+              ['Set-CoOkiE', 'secret'],
+              ['Innocuous', 'hello'],
+            ]},
+          };
+          element._failForCreate200(Promise.resolve(result)).then(() => {
+            assert.isTrue(false, 'Promise should not resolve');
+          }).catch(e => {
+            assert.isOk(e);
+            assert.include(e.message, 'Saving draft resulted in HTTP 200');
+            assert.include(e.message, 'hello');
+            assert.notInclude(e.message, 'secret');
+            done();
+          });
+        });
+
+        test('_failForCreate200 does not fail on 201', done => {
+          const result = {
+            ok: true,
+            status: 201,
+            headers: {entries: () => []},
+          };
+          element._failForCreate200(Promise.resolve(result)).then(() => {
+            done();
+          }).catch(e => {
+            assert.isTrue(false, 'Promise should not fail');
+          });
+        });
       });
     });