List projects api requests

[alukach]

Description

Looking over this diff, this all may be a bit overwhelming.

There were a bunch of small cleanups done along the way, it increasing the logging on the backend to better understand some errors I was experiencing.

The main idea here is to mv any async logic from our components to a separate file. This is don't via redux-thunk. The key to a thunk is that they are dispatch much like normal redux actions are, but they are functions rather than action objects. The functions are called with a dispatch function and the redux state. As they are doing their async operations, they emit other actions. A common workflow for an async operation is to have 3 actions: X_REQUESTED, X_SUCCESS, X_FAILURE. typesafe-actions make it easy to generate these actions.
The core changes in this PR can be found in the following files:

• /projects/projectActions.ts
• /projects/projectReducer.ts
• /project/projectThunks.ts

You can see how we pass in our listProjects thunk as a prop to the List component and then call it when needed: master...list-projects-api-requests#diff-cb3f13aa3b162615cf6e6adf9fb198bbR54 . This way, the component needs to know very little about what it takes to get the project data. Additionally, this data is now stored globally, meaning that it is persisted between view changes and page reloads.

How to test

I found it easiest to test against a deployed API backend. The frontend/src/config.ts is currently set to point at the Dev stack, which is running this branch (at time of writing).

Concerns

I worry that this may overcomplicate the system. I do think that the logic is better separate and allows us to add a number of convenient features to our application (such as persisting data between user sessions), however there's definitely a lot of things to get your head around to feel comfortable with this flow (redux, redux-thunk, typesafe-actions, redux-persist). I expanded the frontend/README.md to try to note these changes, but I would understand if you felt that the complications were not worth the benefits.

Problems

As I was testing these changes, I discovered the following problems. I don't believe that they were caused by this PR, however they may impact your ability to test the API.

CORS errors on backend failures

If communication with the backend fails (e.g. using an expired auth token), the error responses return but are rejected via a CORS exception. You'll see something like the following in your console:

POST https://5mw2vl3rk3.execute-api.us-west-2.amazonaws.com/dev/projects 403 ()

Failed to load https://5mw2vl3rk3.execute-api.us-west-2.amazonaws.com/dev/projects: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://localhost:3000' is therefore not allowed access. The response had HTTP status code 403. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

This is strange, because before the actual POST request is made a pre-flight OPTIONS request is made which returns a 200 response containing the following headers:

access-control-allow-headers: *
access-control-allow-methods: *
access-control-allow-origin: *

You can recreate this error by setting your app's redux state to having a bad token via dispatching the following event within the Redux Dev Tool:

{
    clientId: 'zQxoARkZRA78OUAA',
    username: 'alukach',
    token: 'badToken.',
    tokenExpires: '2018-08-17T03:09:52.873Z',
    portal: 'https://maps.cadasta.org/arcgis/sharing',
    tokenDuration: 20160,
    refreshTokenTTL: 1440,
    _persist: {
      version: -1,
      rehydrated: true
    }
  }

This also occurs if you send an API request without the proper payload (no payload).

Pro-tip If you start getting these CORS errors, look at the response code coming back from the server. Those are still valid. If you're getting a 403, you likely need to refresh your token. If you're getting a 400 or 500, you're likely sending a malformated request.

API Gateway not validating POST body

API Gateway conveniently has a Test feature to test requests made to its endpoints (link). With these tests, we can verify that POST requests made to /projects without the required groups or name parameter return a 400 response with a body of {"message": "Invalid request body"}. The design is that API Gateway will catch the malformed request before ever invoking the Lambda function handler. However, in practice this is actually not true. When the frontend makes an API request that does not conform to the ProjectCreateRequestBodyModel specified in template.yaml, the Lamdba function is run and returns a 500 error (as the request is malformated).

[alukach]

I do note that the frontend tests are failing. I need to adapt it to work with our redux store, will do shortly.

[oliverroick]

I'm still a bit undecided whether it's worth going down the Redux route already or if we should wait until the project gets more complex than it is at the moment. On the other hand, once everything is set up, it seems, that there is overhead in sticking to the Redux pattern but it doesn't seem over complex (and more testable). So maybe it's worth doing it now before we need to refactor a more complex project later. I don't know...

I don't feel overly comfortable assessing your architectural choices as I'm not fluent in React/Redux. I've read up a fair amount during the review and it looks like you've mostly followed best practices so that's ok for me.

What I don't understand yet is the how store and persistor work together. Maybe we can discuss that in a quick call.

What I added

• Fixed a typo in the README
• Added typesafe-actions to package.json

What doesn't work

• The CORS errors are a bit annoying. We're definitely doing something wrong there and we should try to fix it at some point.
• I'm getting random 403 errors. I started the app and logged in and went to the project-list page. Then I went to create a new project but got a 403 error. So I logged out and back in. Now I was able to create a project. Went back to the project list and got the 403 there. I'm not sure if this has to do with the changes you've made or if this is something else...
• The branch needs a rebase and some cleanup to please the linter (bet can't wait for that)

[alukach]

Closes #6

[alukach]

@oliverroick Okay, this has been rebased and fixed up, should be ready to go.

[oliverroick]

There's still an issue with the persisted storage. When I log out, the authentication information is removed from the store. When I reload the page, however, the authentication information is rehydrated from the local storage. I end up with this state:

The logout button on the top is shown, which is incorrect because I just logged out. But the "Sign in" button below is shown correctly. When I click "Sign in" it takes me to the portal authentication page and then updates the token, which leads me to believe, the logout functions correctly, but the old token is persisted even though it shouldn't.

After I did all of this, I created a new project, which worked as expected. If I then go to the project-list page, it only shows the project I just created, but not any of the old projects. When I click on the reload button, I get "Unable to contact backend". So I log out and in again, and the project list loads as expected.

Then I go back to the project-create page, fill the form and submit, I get "Unable to contact backend" here.

I think we should fix this before merging.

[alukach]

@oliverroick Agreed, I noticed this last night as well. If you look at the store (via redux-dev-tools), you can see that the auth state clears to {} but (as you suggested) the local storage is not written to with the update.

[oliverroick]

User credentials were not removed from the persisted store because of a bug in redux-persist. I updated the dependency of redux-persist to the latest version, which fixes the problem.

Another issue, where we were only able to do one operation (either list projects or create projects) and got a 403 for the other operation, was due to the way the authoriser creates the policy. The policy was applied to a specific Lambda ARN (say example-arn/stage/POST/projects, which would allow us to create a project). Since the policy is cached, access to all other endpoints is denied for the next 10 minutes. So instead of granting the policy to example-arn/stage/POST/projects, we need to grant the policy to the full API (example-arn/stage/*/*).

[alukach]

This looks good to me, do you think it's ready to merge?

[oliverroick]

Yes we can go ahead and merge