[Refactor] GroupChoiceModal uses thunk for saving to trellis #1123

ndushay · 2019-08-01T00:36:15Z

Closes #1051

This is a refactor; no functionality change expected.

Please review carefully; I'm pretty sure I got it right but it's my first thunk.

Also wondering if GroupChoiceModal component tests adequately assert the appropriate plumbing methods are called

ndushay

some comments inline

src/components/editor/GroupChoiceModal.jsx

ndushay · 2019-08-01T01:10:43Z

__tests__/actionCreators/resources.test.js

-        },
-      },
-      resource: {
+const state = {


refactoring these test vars out made the other tests shorter and DRYed it up

__tests__/components/editor/GroupChoiceModal.test.js

ndushay · 2019-08-01T01:13:57Z

src/components/editor/GroupChoiceModal.jsx

-      alert('Unable to save resource')
-      console.error('unable to save resource')
-      console.error(err)
-    })
    props.closeRdfPreview()
    props.close()


keeping these modal close actions here made sense to me, as they are pertinent to the logic of the GroupChoiceModal. In fact, I originally closed them before showing the error message and an integration test failed.

src/components/editor/GroupChoiceModal.jsx

jcoyne

This is great. Just a few questions/concerns to wrap up.

__tests__/reducers/index.test.js

jcoyne · 2019-08-01T01:20:38Z

__tests__/reducers/index.test.js

+  it('adds error to editor state', () => {
+    const newState = setPublishError(initialState.selectorReducer, {
+      type: 'RETRIEVE_ERROR',
+      payload: { resourceTemplateId: 'abc123' },


why would it have a resourceTemplateId here? Shouldn't the payload just be a string with the reason?

I was a bit muzzy on this. It can certainly just have the error. Removing reference to non-existent id ...

is there a better way to test these that avoids mocked errors passing?

__tests__/reducers/index.test.js

jcoyne · 2019-08-01T01:23:17Z

src/components/editor/GroupChoiceModal.jsx

-  closeGroupChooser, showRdfPreview, assignBaseURL, showResourceURIMessage,
-  updateStarted, updateFinished,
-} from 'actions/index'
-import { publishRDFResource } from 'sinopiaServer'


src/components/editor/GroupChoiceModal.jsx

jcoyne · 2019-08-01T01:27:55Z

src/reducers/index.js

+//   for now, shameless green.  (Sandi Metz says hold your nose for 3 occurrences before refactoring)
+export const setPublishError = (state, action) => {
+  const resourceTemplateId = action.payload.resourceTemplateId
+  const reason = action.payload.reason


Isn't the payload itself the reason? See https://github.com/LD4P/sinopia_editor/pull/1123/files#diff-4e3037116dbc2bc69a473b7ace3d02abR94

So the action is this, as your link shows:

export const publishError = reason => ({ type: 'PUBLISH_ERROR', payload: reason, })

But that is some notation foo for

export const publishError = reason => ({ type: 'PUBLISH_ERROR', payload: { reason: reason } })

Which I can show empirically by testing diff references in reducer:

const reason = action.payload

results:

while the following passes:

const reason = action.payload?.reason

src/reducers/index.js

ndushay · 2019-08-01T20:07:22Z

@jcoyne @jermnelson the changes I just pushed up should address both of your concerns.

ndushay · 2019-08-01T20:45:28Z

Note that if PR #1111 is merged first, there should be one more update.

jcoyne · 2019-08-01T21:10:08Z

__tests__/reducers/index.test.js

+  it('adds error with reason to editor state', () => {
+    const newState = setPublishError(initialState.selectorReducer, {
+      type: 'PUBLISH_ERROR',
+      payload: { reason: 'publishing error msg' },


I believe the payload here would be a string.

Hrmm. I changed it and got this:

I tried to make it a string before (see comment train above). What am I missing?

__tests__/reducers/index.test.js

src/components/editor/GroupChoiceModal.jsx

src/reducers/index.js

justinlittman · 2019-08-01T23:38:44Z

src/actionCreators/resources.js

+    dispatch(assignBaseURL(resourceUrl))
+    dispatch(showResourceURIMessage(resourceUrl))
+    // Need to regenerate RDF now that we have baseURL
+    const updatedRdf = new GraphBuilder(getState().selectorReducer).graph.toCanonical()


There is going to be a race condition here between line 59 and line 62. If assignBaseURL hasn't completed in time, it will produce an incorrect MD5. I'd recommend putting the base URL in a local copy of state and pass that to the GraphBuilder.

@justinlittman I hear you ... but this is straight copied from what was in code -- this is a refactor PR. Can we do the fix separately, or maybe merge this and put fix in your PR #1111? 🤞

or maybe you can push a commit up to this branch if it's super easy for you?

ndushay · 2019-08-01T23:48:02Z

@jcoyne @justinlittman more changes ... are we done yet? :-)

…t (by Editor)

ndushay · 2019-08-02T16:38:55Z

src/actionCreators/resources.js

+
+  return publishRDFResource(currentUser, rdf, group).then((result) => {
+    const resourceUrl = result.response.headers.location
+    dispatch(assignBaseURL(resourceUrl))


is this assignBaseURL dispatch still appropriate given lines 65-66

ndushay added BFF labels Aug 1, 2019

ndushay added this to Needs Review in Sinopia Work-Cycle One via automation Aug 1, 2019

ndushay commented Aug 1, 2019

View reviewed changes

jcoyne requested changes Aug 1, 2019

View reviewed changes

jermnelson requested changes Aug 1, 2019

View reviewed changes

src/reducers/index.js Outdated Show resolved Hide resolved

jermnelson approved these changes Aug 1, 2019

View reviewed changes

ndushay mentioned this pull request Aug 1, 2019

Fixes setting last save checksum so that it is called sync rather tha… #1111

Merged

jcoyne requested changes Aug 1, 2019

View reviewed changes

justinlittman requested changes Aug 1, 2019

View reviewed changes

ndushay changed the title ~~GroupChoiceModal uses thunk for saving to trellis~~ [Refactor] GroupChoiceModal uses thunk for saving to trellis Aug 1, 2019

ndushay force-pushed the group-choice-thunk branch from 0dbdd7d to 22b8f27 Compare August 1, 2019 23:47

ndushay and others added 10 commits August 2, 2019 08:14

reducers/index: update comment for consistency

26f3922

reducer clearServerError new name (was clearRetrieveError)

a6712d2

add publishStarted, publishError actions

ce3f243

reducer for PUBLISH_ERROR, PUBLISH_STARTED

4bae1bb

add thunk to publish (save) a new resource to Trellis

db7907c

GroupChoiceModal: use publishResource thunk

1e4b698

reducers/index and its tests: improvements per PR comments

e19b1e3

GroupChoiceModal: publish error displayed with ErrorMessages componen…

a52ed36

…t (by Editor)

GroupChoiceModal thunk refactor: improvements per PR comments

562b137

Avoid race condition setting base url.

7f0f9b2

justinlittman force-pushed the group-choice-thunk branch from e8b3ac6 to 7f0f9b2 Compare August 2, 2019 12:37

justinlittman approved these changes Aug 2, 2019

View reviewed changes

jcoyne merged commit 38c481f into master Aug 2, 2019

Sinopia Work-Cycle One automation moved this from Needs Review to Done Aug 2, 2019

jcoyne deleted the group-choice-thunk branch August 2, 2019 13:00

ndushay commented Aug 2, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] GroupChoiceModal uses thunk for saving to trellis #1123

[Refactor] GroupChoiceModal uses thunk for saving to trellis #1123

ndushay commented Aug 1, 2019 •

edited

ndushay left a comment

ndushay Aug 1, 2019

ndushay Aug 1, 2019

jcoyne left a comment

jcoyne Aug 1, 2019

ndushay Aug 1, 2019 •

edited

ndushay Aug 1, 2019 •

edited

jcoyne Aug 1, 2019

jcoyne Aug 1, 2019

ndushay Aug 1, 2019

ndushay commented Aug 1, 2019

ndushay commented Aug 1, 2019

jcoyne Aug 1, 2019

ndushay Aug 1, 2019

justinlittman Aug 1, 2019

ndushay Aug 1, 2019

ndushay Aug 1, 2019

ndushay commented Aug 1, 2019

ndushay Aug 2, 2019

[Refactor] GroupChoiceModal uses thunk for saving to trellis #1123

[Refactor] GroupChoiceModal uses thunk for saving to trellis #1123

Conversation

ndushay commented Aug 1, 2019 • edited

ndushay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay Aug 1, 2019 • edited

Choose a reason for hiding this comment

ndushay Aug 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay commented Aug 1, 2019

ndushay commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay commented Aug 1, 2019

Choose a reason for hiding this comment

ndushay commented Aug 1, 2019 •

edited

ndushay Aug 1, 2019 •

edited

ndushay Aug 1, 2019 •

edited