Add deleteComment() API and support DELETE responses better (#1115)

src/api/index.spec.tsx

@@ -8,12 +8,14 @@ import {
 import {
   createFakeCommentsResponse,
   createFakeExternalComment,
+  setMockFetchResponseJSON,
 } from '../test-helpers';
 
 import {
   HttpMethod,
   callApi,
   createOrUpdateComment,
+  deleteComment,
   getApiHost,
   getComments,
   getCurrentUser,
@@ -169,7 +171,56 @@ describe(__filename, () => {
 
     it('returns an object containing the API response', async () => {
       const data = { some: 'data', count: 123 };
-      fetchMock.mockResponse(JSON.stringify(data));
+      setMockFetchResponseJSON(data);
+
+      const response = await callApiWithDefaultApiState();
+
+      expect(response).toEqual(data);
+    });
+
+    it('assumes a text response without content-type', async () => {
+      const serverResponse = 'some kind of text response';
+      fetchMock.mockResponse(serverResponse);
+
+      const response = await callApiWithDefaultApiState();
+
+      expect(response).toEqual(serverResponse);
+    });
+
+    it('handles text with an explicit content-type', async () => {
+      const serverResponse = 'some kind of text response';
+      fetchMock.mockResponse(serverResponse, {
+        headers: { 'content-type': 'text/plain' },
+      });
+
+      const response = await callApiWithDefaultApiState();
+
+      expect(response).toEqual(serverResponse);
+    });
+
+    it('handles JSON with an explicit content-type', async () => {
+      const data = { thing: 'value' };
+      setMockFetchResponseJSON(data, { contentType: 'application/json' });
+
+      const response = await callApiWithDefaultApiState();
+
+      expect(response).toEqual(data);
+    });
+
+    it('handles JSON with a content-type specifying the encoding', async () => {
+      const data = { thing: 'value' };
+      setMockFetchResponseJSON(data, {
+        contentType: 'application/json; charset=utf-8',
+      });
+
+      const response = await callApiWithDefaultApiState();
+
+      expect(response).toEqual(data);
+    });
+
+    it('treats a JSON content-type as case-insensitive', async () => {
+      const data = { thing: 'value' };
+      setMockFetchResponseJSON(data, { contentType: 'APPLICATION/JSON' });
 
       const response = await callApiWithDefaultApiState();
 
@@ -726,4 +777,32 @@ describe(__filename, () => {
       });
     });
   });
+
+  describe('deleteComment', () => {
+    it('deletes a comment', async () => {
+      const addonId = 1;
+      const apiState = defaultApiState;
+      const commentId = 2;
+      const versionId = 3;
+
+      const serverResponse = '';
+      const _callApi = jest.fn().mockResolvedValue(serverResponse);
+
+      const response = await deleteComment({
+        _callApi,
+        addonId,
+        commentId,
+        apiState,
+        versionId,
+      });
+
+      expect(response).toEqual(serverResponse);
+
+      expect(_callApi).toHaveBeenCalledWith({
+        apiState,
+        endpoint: `reviewers/addon/${addonId}/versions/${versionId}/draft_comments/${commentId}`,
+        method: HttpMethod.DELETE,
+      });
+    });
+  });
 });

src/api/index.tsx

@@ -137,11 +137,16 @@ type Headers = {
   [name: string]: string;
 };
 
-type GetResource<SuccessfulResponse> = {
+type ResponseOnly<SuccessfulResponse> = {
   requestData: undefined;
   successfulResponse: SuccessfulResponse;
 };
 
+type EmptyRequestAndResponse = {
+  requestData: undefined;
+  successfulResponse: '';
+};
+
 export const callApi = async <
   T extends {
     requestData: undefined | {};
@@ -202,7 +207,14 @@ export const callApi = async <
       );
     }
 
-    return await response.json();
+    const contentType = (
+      response.headers.get('content-type') || ''
+    ).toLowerCase();
+
+    return await (contentType.startsWith('application/json')
+      ? response.json()
+      : // The API might return a text response, like 204s.
+        response.text());
   } catch (error) {
     log.debug('Error caught in callApi():', error);
 
@@ -232,7 +244,7 @@ export const getVersion = async ({
   addonId,
   versionId,
 }: GetVersionParams) => {
-  return callApi<GetResource<ExternalVersionWithContent>>({
+  return callApi<ResponseOnly<ExternalVersionWithContent>>({
     apiState,
     endpoint: `reviewers/addon/${addonId}/versions/${versionId}`,
     query: path ? { file: path } : undefined,
@@ -248,14 +260,14 @@ export const getVersionsList = async ({
   apiState,
   addonId,
 }: GetVersionsListParams) => {
-  return callApi<GetResource<ExternalVersionsList>>({
+  return callApi<ResponseOnly<ExternalVersionsList>>({
     apiState,
     endpoint: `reviewers/addon/${addonId}/versions/`,
   });
 };
 
 export const logOutFromServer = async (apiState: ApiState) => {
-  return callApi<GetResource<{}>>({
+  return callApi<ResponseOnly<{ ok: boolean }>>({
     apiState,
     // We need to send the credentials (cookies) because the API will return
     // new `Set-Cookie` headers to clear the cookies in the client. Without
@@ -267,7 +279,7 @@ export const logOutFromServer = async (apiState: ApiState) => {
 };
 
 export const getCurrentUser = async (apiState: ApiState) => {
-  return callApi<GetResource<ExternalUser>>({
+  return callApi<ResponseOnly<ExternalUser>>({
     apiState,
     endpoint: '/accounts/profile/',
   });
@@ -288,7 +300,7 @@ export const getDiff = async ({
   headVersionId,
   path,
 }: GetDiffParams) => {
-  return callApi<GetResource<ExternalVersionWithDiff>>({
+  return callApi<ResponseOnly<ExternalVersionWithDiff>>({
     apiState,
     endpoint: `reviewers/addon/${addonId}/versions/${baseVersionId}/compare_to/${headVersionId}`,
     query: path ? { file: path } : undefined,
@@ -365,9 +377,29 @@ export const getComments = async ({
   apiState: ApiState;
   versionId: number;
 }) => {
-  return _callApi<GetResource<GetCommentsResponse>>({
+  return _callApi<ResponseOnly<GetCommentsResponse>>({
     apiState,
     endpoint: `reviewers/addon/${addonId}/versions/${versionId}/draft_comments`,
     method: HttpMethod.GET,
   });
 };
+
+export const deleteComment = async ({
+  _callApi = callApi,
+  addonId,
+  commentId,
+  apiState,
+  versionId,
+}: {
+  _callApi?: typeof callApi;
+  addonId: number;
+  apiState: ApiState;
+  commentId: number;
+  versionId: number;
+}) => {
+  return _callApi<EmptyRequestAndResponse>({
+    apiState,
+    endpoint: `reviewers/addon/${addonId}/versions/${versionId}/draft_comments/${commentId}`,
+    method: HttpMethod.DELETE,
+  });
+};

src/reducers/linter.spec.tsx

@@ -4,6 +4,7 @@ import {
   createFakeLinterMessagesByPath,
   fakeExternalLinterResult,
   fakeExternalLinterMessage,
+  setMockFetchResponseJSON,
   thunkTester,
 } from '../test-helpers';
 import linterReducer, {
@@ -436,7 +437,7 @@ describe(__filename, () => {
       respondWithResult?: boolean;
     } = {}) => {
       if (respondWithResult) {
-        fetchMock.mockResponse(JSON.stringify(result));
+        setMockFetchResponseJSON(result);
       }
 
       return thunkTester({

src/test-helpers.tsx

@@ -1,4 +1,5 @@
 /* eslint @typescript-eslint/camelcase: 0 */
+/* global fetchMock */
 import pathLib from 'path';
 
 import * as React from 'react';
@@ -931,3 +932,15 @@ export const createFakeCommentsResponse = (
 ) => {
   return createFakeApiPage<GetCommentsResponse>({ results });
 };
+
+export const setMockFetchResponseJSON = (
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  data: Record<string, any>,
+  { contentType = 'application/json' } = {},
+) => {
+  fetchMock.mockResponse(JSON.stringify(data), {
+    headers: {
+      'content-type': contentType,
+    },
+  });
+};