Connect the frontend UI with the backend API #56

dpordomingo · 2018-02-01T11:51:17Z

List of changes:

c029aba Make Assignment.Answer nullable
901345b Fix push FilePair api url
b603e55 Use backend instead of mocks in front
371c146 Implement the answering from UI

Other minor changes:

f2a0535 Emit valid diffs
0755005 Use answers as defined by the Backend

smacker · 2018-02-01T17:41:36Z

src/api/index.js

  const token = TokenService.get();

+  if (options.body) {
+    options.body = JSON.stringify(options.body)


content-type is missed

smacker · 2018-02-01T17:42:11Z

src/api/index.js

-  };
-}

 // eslint-disable-next-line


no need for it anymore

I added it because there were mocks.
Now we can safely remove it.
Please take a look how I did it when connected API to UI:
smacker@7340215#diff-284817f1b6aad47c0ac6d61bd72f474c

smacker · 2018-02-01T17:43:18Z

src/state/experiment.js

  );
 };

+const pushAnswer = (experimentId, assignmentId, answer) => (dispatch, getState) => {


it's not push, it's put or update :)

smacker · 2018-02-01T17:46:18Z

src/state/experiment.js

+  }
+  return api.putAnswer(experimentId, assignmentId, answerPayload).catch(e => {
+    dispatch(addErrors(e));
+    dispatch({ type: LOAD_ERROR, error: e });


it will be super weird behavior and also race condition is possible

I copied it from other places

code-annotation/src/state/experiment.js

Line 136 in b2e1db2

dispatch(addErrors(e));

code-annotation/src/state/experiment.js

Line 179 in b2e1db2

dispatch(addErrors(e));

Did I miss something? Any dispatch maybe?

Yes, you are missing the logic. Of messed up with how async promises work.
Let me explain the whole flow:

A user opens page /exp/1/1, what happens next:

middleware see the change and call selectAssigment action

selectAssigment sets assignmentId and calls loadFilePairIfNeeded action

If there are no files in cache, it calls API and returns promise

When promise is resolved it sets file content to store or if promise is rejected it sets error

User choose are those files similar or different. When user clicks button it calls mark action.

mark action sets answer to store

next your changes that work wrong

You call pushAnswer action, it returns promise and sets error

at the same time (next tick) it calls nextAssigment which starts changes url and everything starts over.

It's very possible that next assignment will be loaded and shown to user before PUT api call has finished. What would happen in such case?

User will see next assignment and start deciding answer for the next assignment

Then he will get error at very random time

right now we don't reset errors anywhere but if we do later it will be even worst.
Because you don't wait for the promise, another promise can remove your error.
It will look like this for user:

Click button

Next assignment loader

Some weird flash

Next assignment, but the previous answer wasn't saved on server

smacker · 2018-02-01T17:47:05Z

error handling is missed also. Take a look here: smacker@7340215#diff-284817f1b6aad47c0ac6d61bd72f474c

dpordomingo · 2018-02-02T12:28:28Z

@smacker feedback addressed by 29121ae

about error handling from #56 (comment)
I didn't want to do more things than required. I just added what it was needed to connect with the backend API regardless previous state (that didn't handle errors).
I think it should be better do it in a separated PR as a global thing, shouldn't be?

smacker · 2018-02-02T13:05:56Z

In my opinion correct api handler is part of connection issue. You connect not only successful case but unsuccessful too. But if you prefer to do it in another PR it’s fine.

bzz · 2018-02-06T10:24:11Z

@dpordomingo could you please help me to understand, what is the status of this PR?

There seems to be a CI failure and some feedback may still need to be addressed.

dpordomingo · 2018-02-06T11:44:06Z

@bzz CI success, but this is still pending #56 (comment).

@smacker do you think it should be fixed before merging?
If possible, I'd merge and continue working in that (that way, @carlosms could rebase its work from #52 on top of this one, and test its implementation with a real working site)
If you think we can merge without #56 (comment) tell me, and I'll create an issue, rebase, merge, and continue working in that request.

smacker · 2018-02-07T14:38:08Z

@dpordomingo current code doesn't work correctly. It's not just missed feature it's a bug.

To fix it just change:

dispatch(pushAnswer(experimentId, assignmentId, answer));
return dispatch(nextAssigment());

return dispatch(pushAnswer(experimentId, assignmentId, answer)).then(() => {
  return dispatch(nextAssigment());
});

there can be a smarter solution, but this works and good enough to allow PR get merged.

@smacker

Added during review stage - Simplify export as suggested by @smacker - Add 'Content-type' and rename putAnswer func as requested by @smacker

dpordomingo · 2018-02-08T17:33:30Z

Many thanks @smacker for the hint you offered by #56 (comment) ; I added it here: d2522bd

I also had to fix things after rebasing agains current master:

c16121f Add default internal db to the .gitignore as agreed with @carlosms
737db98 Implement Scanner and Valuer for model.Role as explained irl to @carlosms and @smacker
71b2e65 Return 404 error when no assignments available to be labeled

bzz · 2018-02-09T09:38:23Z

server/model/models.go


+// Value returns the string value of the Role
+func (r Role) Value() (driver.Value, error) {
+	// TODO: role validation (@dpordomingo)


Usual way of formatting todos is

//TODO(dpordomingo): role validation

smacker · 2018-02-09T10:19:36Z

server/model/models.go

+
+	var bv driver.Value
+	var err error
+	if bv, err = driver.String.ConvertValue(value); err == nil {


why so complicated? https://play.golang.org/p/3ynyW8ZGKJ5 covers everything we need, no?

smacker · 2018-02-09T10:24:42Z

server/handler/assignments.go

 		assignments, err := repo.GetAll(userID, experimentID)
 		if err == repository.ErrNoAssignmentsInitialized {
 			if assignments, err = repo.Initialize(userID, experimentID); err != nil {
+				if err == repository.ErrNoPairsAvailable {


it's incorrect if we are following REST. List APIs should return an empty list when nothing is found, but a request is correct. There is no special rule for it because actually spec for REST doesn't exist. But if you look at any REST API (github for example) you will notice it works like that.

404 error for endpoint /api/:id/list means there is no such API or id isn't found. Not empty list.

/api/experiments/{experimentId}/assignments is requesting for the assignments of a given experiment, so this 404 Not Found means that the collection does not exist at all. It is not an "empty" case.

About the status code: 404 Not Found it's defined as

The requested resource could not be found but may be available in the future. Subsequent requests by the client are permissible.

what matches our case: if in the future are loaded file-pairs, the endpoint will return data.

smacker · 2018-02-09T10:27:32Z

src/api/index.js

+  );
 }

 // eslint-disable-next-line


as I mentioned before we don't need to disable eslint anymore.

smacker

please don't break common REST conventions.

dpordomingo · 2018-02-09T13:26:53Z

@bzz @smacker Your suggestions were addressed by 2623b4f
I didn't change the response to an unexistent collection of assignments because since the collection does not exist, it makes sense the 404 (it is not an empty collection, but a collection that does not exist)
I think that further discussions should be addressed aside.

dpordomingo · 2018-02-09T16:18:30Z

I don't think it worths that amount of discussions for something already done, that is consistent with REST and HTTP status codes and fits our necessities.

Anyhow we need to go prod as soon as possible and it's better to move on, so I dropped the ErrNoPairsAvailable error 65c4e0a.
The amount of code removed is not relevant at all, and now the frontend also needs changes, but it can be done in a separated PR:

When the user has no assignments to fill, instead of rising a reasonable error, he obtains a "success for filling NaN% experiments, with no possibility to move to any other window.

issue: #82

smacker

Looks great now! Let's merge!!!!

bzz

LGTM

Awesome ✨ !

dpordomingo · 2018-02-09T18:15:05Z

😢 I wanted to rebase and reorder de history...

dpordomingo requested review from bzz and smacker February 1, 2018 11:51

dpordomingo force-pushed the frontend-api branch from fb1da3b to 371c146 Compare February 1, 2018 12:41

smacker suggested changes Feb 1, 2018

View reviewed changes

dpordomingo force-pushed the frontend-api branch from 5ba4cdc to 29121ae Compare February 2, 2018 12:29

dpordomingo force-pushed the frontend-api branch from 29121ae to b3df05c Compare February 2, 2018 15:03

dpordomingo force-pushed the frontend-api branch from b3df05c to e23350a Compare February 6, 2018 11:06

carlosms mentioned this pull request Feb 6, 2018

Use DB in the backend #52

Merged

2 tasks

dpordomingo mentioned this pull request Feb 7, 2018

Prepare first release of code annotation #70

Closed

16 tasks

bzz mentioned this pull request Feb 8, 2018

Backend: REST API #3

Closed

8 tasks

dpordomingo added 2 commits February 8, 2018 17:41

Fix push FilePair api url

85f2685

Use backend API in front instead of mocks

b24909a

Added during review stage - Simplify export as suggested by @smacker - Add 'Content-type' and rename putAnswer func as requested by @smacker

dpordomingo force-pushed the frontend-api branch from 1e32a95 to f58ec52 Compare February 8, 2018 17:30

dpordomingo added 4 commits February 8, 2018 18:37

Fix race condition when user answers as spotted by @smacker

d2522bd

Add default internal db to the .gitignore

c16121f

Implement Scanner and Valuer for model.Role

737db98

Return 404 error when no assignments available to be labeled

71b2e65

dpordomingo force-pushed the frontend-api branch from f58ec52 to 71b2e65 Compare February 8, 2018 17:38

dpordomingo requested a review from smacker February 8, 2018 20:40

bzz assigned dpordomingo Feb 9, 2018

bzz reviewed Feb 9, 2018

View reviewed changes

smacker reviewed Feb 9, 2018

View reviewed changes

smacker suggested changes Feb 9, 2018

View reviewed changes

Address @bzz and @smacker comments

2623b4f

Return 200:[] when no assignments ready

65c4e0a

dpordomingo force-pushed the frontend-api branch from cf13268 to 65c4e0a Compare February 9, 2018 16:19

smacker approved these changes Feb 9, 2018

View reviewed changes

dpordomingo requested a review from bzz February 9, 2018 16:35

bzz approved these changes Feb 9, 2018

View reviewed changes

bzz merged commit 0b0b404 into src-d:master Feb 9, 2018

dpordomingo mentioned this pull request Feb 9, 2018

Fix message when no assignments available to an experiment #82

Closed

dpordomingo deleted the frontend-api branch February 9, 2018 18:25

Connect the frontend UI with the backend API #56

Connect the frontend UI with the backend API #56

Uh oh!

Conversation

dpordomingo commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smacker commented Feb 1, 2018

Uh oh!

dpordomingo commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacker commented Feb 2, 2018

Uh oh!

bzz commented Feb 6, 2018

Uh oh!

dpordomingo commented Feb 6, 2018 • edited by bzz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

dpordomingo commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

dpordomingo commented Feb 9, 2018

Uh oh!

dpordomingo commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

bzz left a comment

Choose a reason for hiding this comment

Uh oh!

dpordomingo commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dpordomingo commented Feb 1, 2018 •

edited

Loading

dpordomingo commented Feb 2, 2018 •

edited

Loading

dpordomingo commented Feb 6, 2018 •

edited by bzz

Loading

dpordomingo commented Feb 8, 2018 •

edited

Loading

dpordomingo commented Feb 9, 2018 •

edited

Loading