From afc9e9032e285e506941b3510e60ded139c66a66 Mon Sep 17 00:00:00 2001 From: "Michael J. Giarlo" Date: Thu, 23 May 2019 09:00:42 -0700 Subject: [PATCH] Prevent upload modal & flash message from showing stale messages Fixes #534 This branch includes the following changes: * Reset messages and errors in `ImportResourceTemplate` component when HTTP response is successful * Reset messages when update handler is finished * Prevent users from dismissing flash message box, else it never comes back * Extract code to parse a response object into a human-friendly location into a new function in `ImportResourceTemplate` component * Add Babel plugin for optional chaining (as dev dependency) via the safe navigation (or "Elvis") operator * This lets us safely reach into objects such as HTTP responses without having to check at every level for `null` or `undefined` --- .babelrc | 3 +- .../editor/ImportResourceTemplate.test.js | 89 ++++++++++++++++++- package-lock.json | 19 ++++ package.json | 1 + .../editor/ImportResourceTemplate.jsx | 36 ++++++-- .../editor/SinopiaResourceTemplates.jsx | 3 +- 6 files changed, 138 insertions(+), 13 deletions(-) diff --git a/.babelrc b/.babelrc index d9d21b14e..59792dc10 100644 --- a/.babelrc +++ b/.babelrc @@ -5,6 +5,7 @@ ], "plugins": [ "@babel/plugin-proposal-class-properties", - "@babel/plugin-transform-runtime" + "@babel/plugin-transform-runtime", + "@babel/plugin-proposal-optional-chaining" ] } diff --git a/__tests__/components/editor/ImportResourceTemplate.test.js b/__tests__/components/editor/ImportResourceTemplate.test.js index 12f63d63d..6021b4bdc 100644 --- a/__tests__/components/editor/ImportResourceTemplate.test.js +++ b/__tests__/components/editor/ImportResourceTemplate.test.js @@ -124,10 +124,11 @@ describe('', () => { expect(wrapper.state().message).toEqual([]) - wrapper.instance().updateStateFromServerResponse({ status: 200, headers: {} }) + wrapper.instance().updateStateFromServerResponse({ status: 200, headers: {} }, 0) expect(wrapper.state().message).toEqual(['Unexpected response (200)! ']) }) + it('sets modalShow to true when receiving HTTP 409 and errors >= profileCount', () => { // Set new wrapper in each of these tests because we are changing state wrapper = shallow() @@ -135,17 +136,93 @@ describe('', () => { expect(wrapper.state().modalShow).toBe(false) wrapper.instance().updateStateFromServerResponse({ status: 409 , headers: {} }, 0) + expect(wrapper.state().modalShow).toBe(true) }) + it('sets message in state with any create operation not resulting in HTTP 409', () => { // Set new wrapper in each of these tests because we are changing state wrapper = shallow() expect(wrapper.state().message).toEqual([]) - wrapper.instance().updateStateFromServerResponse({ status: 201 , headers: { location: 'http://foo.bar' } }) + wrapper.instance().updateStateFromServerResponse({ status: 201 , headers: { location: 'http://sinopia.io/repository/ld4p/myResourceTemplate' } }, 0) + + expect(wrapper.state().message).toEqual(['Created http://sinopia.io/repository/ld4p/myResourceTemplate']) + }) + + it('resets createResourceError and message in component state when response is OK', () => { + // Set new wrapper in each of these tests because we are changing state + wrapper = shallow() + + wrapper.setState({ createResourceError: ['Could not update resource!'], message: ['Updated http://sinopia.io/repository/ld4p/myResourceTemplate'] }) + expect(wrapper.state().createResourceError).toEqual(['Could not update resource!']) + expect(wrapper.state().message).toEqual(['Updated http://sinopia.io/repository/ld4p/myResourceTemplate']) + + wrapper.instance().updateStateFromServerResponse({ ok: true, status: 204 }, 0) - expect(wrapper.state().message).toEqual(['Created http://foo.bar']) + expect(wrapper.state().createResourceError).toEqual([]) + expect(wrapper.state().message).toEqual(['Updated ']) + }) + + it('leaves createResourceError alone and appends message in component state when response is not OK', () => { + // Set new wrapper in each of these tests because we are changing state + wrapper = shallow() + + wrapper.setState({ createResourceError: ['Could not update resource!'], message: ['Updated http://sinopia.io/repository/ld4p/myResourceTemplate'] }) + expect(wrapper.state().createResourceError).toEqual(['Could not update resource!']) + expect(wrapper.state().message).toEqual(['Updated http://sinopia.io/repository/ld4p/myResourceTemplate']) + + wrapper.instance().updateStateFromServerResponse({ ok: false, status: 400 }, 0) + + expect(wrapper.state().createResourceError).toEqual(['Could not update resource!']) + expect(wrapper.state().message).toEqual(['Updated http://sinopia.io/repository/ld4p/myResourceTemplate', 'Unexpected response (400)! ']) + }) + }) + + describe('humanReadableLocation()', () => { + it('returns human-readable label when Location header exists', () => { + const response = { + headers: { + location: 'http://sinopia.io/repository/ld4p/myResourceTemplate' + }, + req: { + url: 'http://sinopia.io/repository/ld4p', + _data: { + id: 'myResourceTemplate' + } + } + } + expect(wrapper.instance().humanReadableLocation(response)).toEqual('http://sinopia.io/repository/ld4p/myResourceTemplate') + }) + + it('returns human-readable label when data ID exists', () => { + const response = { + req: { + url: 'http://sinopia.io/repository/ld4p', + _data: { + id: 'myResourceTemplate' + } + } + } + expect(wrapper.instance().humanReadableLocation(response)).toEqual('http://sinopia.io/repository/ld4p/myResourceTemplate') + }) + + it('returns human-readable label when request URL already includes data ID', () => { + const response = { + req: { + url: 'http://sinopia.io/repository/ld4p/myResourceTemplate', + _data: { + id: 'myResourceTemplate' + } + } + } + expect(wrapper.instance().humanReadableLocation(response)).toEqual('http://sinopia.io/repository/ld4p/myResourceTemplate') + }) + + it('returns an empty label otherwise', () => { + const response = undefined + expect(wrapper.instance().humanReadableLocation(response)).toEqual('') }) }) @@ -153,15 +230,19 @@ describe('', () => { it('returns human-readable label for HTTP 201', () => { expect(wrapper.instance().humanReadableStatus(201)).toEqual('Created') }) + it('returns human-readable label for HTTP 204', () => { expect(wrapper.instance().humanReadableStatus(204)).toEqual('Updated') }) + it('returns human-readable label for HTTP 401', () => { expect(wrapper.instance().humanReadableStatus(401)).toEqual('You are not authorized to do this. Try logging in again!') }) + it('returns human-readable label for HTTP 409', () => { expect(wrapper.instance().humanReadableStatus(409)).toEqual('Attempting to update') }) + it('returns human-readable label for any other HTTP status', () => { expect(wrapper.instance().humanReadableStatus(400)).toEqual('Unexpected response (400)!') }) @@ -181,11 +262,13 @@ describe('', () => { const updateResourceSpy = jest.spyOn(wrapper.instance(), 'updateResource').mockImplementation(async () => {}) const updateStateSpy = jest.spyOn(wrapper.instance(), 'updateStateFromServerResponse').mockReturnValue(null) const modalCloseSpy = jest.spyOn(wrapper.instance(), 'modalClose').mockReturnValue(null) + const setStateSpy = jest.spyOn(wrapper.instance(), 'setState').mockReturnValue(null) await wrapper.instance().handleUpdateResource(templates, 'ld4p') expect(updateResourceSpy).toHaveBeenCalledTimes(2) expect(updateStateSpy).toHaveBeenCalledTimes(2) + expect(setStateSpy).toHaveBeenNthCalledWith(1, { message: [] }) expect(modalCloseSpy).toHaveBeenCalledTimes(1) }) }) diff --git a/package-lock.json b/package-lock.json index fa37e9711..736ca8e46 100644 --- a/package-lock.json +++ b/package-lock.json @@ -818,6 +818,16 @@ "@babel/plugin-syntax-optional-catch-binding": "^7.2.0" } }, + "@babel/plugin-proposal-optional-chaining": { + "version": "7.2.0", + "resolved": "https://registry.npmjs.org/@babel/plugin-proposal-optional-chaining/-/plugin-proposal-optional-chaining-7.2.0.tgz", + "integrity": "sha512-ea3Q6edZC/55wEBVZAEz42v528VulyO0eir+7uky/sT4XRcdkWJcFi1aPtitTlwUzGnECWJNExWww1SStt+yWw==", + "dev": true, + "requires": { + "@babel/helper-plugin-utils": "^7.0.0", + "@babel/plugin-syntax-optional-chaining": "^7.2.0" + } + }, "@babel/plugin-proposal-unicode-property-regex": { "version": "7.4.4", "resolved": "https://registry.npmjs.org/@babel/plugin-proposal-unicode-property-regex/-/plugin-proposal-unicode-property-regex-7.4.4.tgz", @@ -874,6 +884,15 @@ "@babel/helper-plugin-utils": "^7.0.0" } }, + "@babel/plugin-syntax-optional-chaining": { + "version": "7.2.0", + "resolved": "https://registry.npmjs.org/@babel/plugin-syntax-optional-chaining/-/plugin-syntax-optional-chaining-7.2.0.tgz", + "integrity": "sha512-HtGCtvp5Uq/jH/WNUPkK6b7rufnCPLLlDAFN7cmACoIjaOOiXxUt3SswU5loHqrhtqTsa/WoLQ1OQ1AGuZqaWA==", + "dev": true, + "requires": { + "@babel/helper-plugin-utils": "^7.0.0" + } + }, "@babel/plugin-transform-arrow-functions": { "version": "7.2.0", "resolved": "https://registry.npmjs.org/@babel/plugin-transform-arrow-functions/-/plugin-transform-arrow-functions-7.2.0.tgz", diff --git a/package.json b/package.json index 3539c8dfc..d57b6a2fa 100755 --- a/package.json +++ b/package.json @@ -22,6 +22,7 @@ "@babel/core": "^7.4.3", "@babel/node": "^7.4.5", "@babel/plugin-proposal-class-properties": "^7.4.0", + "@babel/plugin-proposal-optional-chaining": "^7.2.0", "@babel/plugin-transform-runtime": "^7.4.3", "@babel/preset-env": "^7.4.3", "@babel/preset-react": "^7.0.0", diff --git a/src/components/editor/ImportResourceTemplate.jsx b/src/components/editor/ImportResourceTemplate.jsx index 88128a0f5..43dbf1e36 100644 --- a/src/components/editor/ImportResourceTemplate.jsx +++ b/src/components/editor/ImportResourceTemplate.jsx @@ -85,20 +85,39 @@ class ImportResourceTemplate extends Component { updateStateFromServerResponse = (response, profileCount, updateKey) => { // HTTP status 409 == Conflict const showModal = response.status === 409 && this.state.createResourceError.length >= profileCount - - const location = response.headers.location || '' - const newMessage = `${this.humanReadableStatus(response.status)} ${location}` - const newState = { - message: [...this.state.message, newMessage], - updateKey: updateKey - } + const newMessage = `${this.humanReadableStatus(response.status)} ${this.humanReadableLocation(response)}` + const newState = { updateKey: updateKey } if (showModal) newState.modalShow = true + if (response.ok) { + // Wipe out past messages and errors if response was successful + newState.createResourceError = [] + newState.message = [newMessage] + } else { + newState.message = [...this.state.message, newMessage] + } + this.setState(newState) } + // Returns a URL or an empty string + humanReadableLocation = response => { + if (response?.headers?.location) + return response.headers.location + + if (response?.req?._data?.id) { + // If the request URL already contains the ID, don't bother appending + if (response.req.url.endsWith(response.req._data.id)) + return response.req.url + return `${response.req.url}/${response.req._data.id}` + } + + // Welp, we tried anyway + return '' + } + humanReadableStatus = status => { switch(status) { case 201: @@ -121,6 +140,9 @@ class ImportResourceTemplate extends Component { // The 0 is the profileCount which is only used for tracking create errors this.updateStateFromServerResponse(response, 0, incrementedKey) }) + this.setState({ + message: [] + }) this.modalClose() } diff --git a/src/components/editor/SinopiaResourceTemplates.jsx b/src/components/editor/SinopiaResourceTemplates.jsx index 15a02d600..703a09d05 100644 --- a/src/components/editor/SinopiaResourceTemplates.jsx +++ b/src/components/editor/SinopiaResourceTemplates.jsx @@ -118,8 +118,7 @@ class SinopiaResourceTemplates extends Component { ) } - let createResourceMessage =
- + let createResourceMessage =
{ this.props.message.join(', ') }
;