Migrate API controllers to lumen #1674

rjmackay · 2017-04-06T00:04:14Z

This pull request makes the following changes:

Test these changes by:

Run tests

Ping @ushahidi/platform

This change is

- Create base REST Controller w/ pre + post processing - Call usecases - Make errors JSON if the request asks for JSON - Minor fixes to REST controller - Add CORS Middleware to set headers - Add config controller to test with

- Hook up auth to our UserRepo - Add migrations for passport - Connect Passport to Ushahidi Users so that auth actually works!

- Add posts, revisions, translations APIs - Add export and geojson api - Fix up RestContext issues in testing - Excluding updates API - Still need to fix up validation error output

- Make sure we're outputting JSON errors - No longer accepting access token via query param - Return 401 not 400 for missing access token - Return 401 not 400 for incorrect password

- Remove custom CORS middleware - Add tests for preflight requests - Add tests for CORS requests - Add tests for vanilla OPTIONS requests - Add routes for /config and /posts OPTIONS requests

rjmackay · 2017-05-12T01:33:53Z

@willdoran given this is kinda huge heres the minimum that I think you need to review:

Double check my changes to the tests
Review the code in app/ outside of Http Controllers
- Particularly check the wiring around Passport/auth
Review 1 or 2 controllers. Maybe look at the special cases for Post and Forms
Review the base RESTController

willdoran · 2017-05-12T15:20:28Z

@rjmackay It all looks like it's coming along, I don't see anything wrong, the tests look good generally. I like the directory structure generally, though I wonder about whether we could adopt the approach of moving the tests right beside the code. I found that structure very effective in the rollcall js client. Though it may not be strictly kosher in the lumen structure.

My main concern looking at the Repos is that our data model needs to be restructured specifically for forms, stages, attributes and posts. If nothing else we should take the chance to rename them to match the client. That is probably true for Tags as well.

May be in Ireland we can updatet the data model. I think involving Dale in that would be good. By then we'll have basically 3/3.5 clients(mobile, facebot, comrades, client) and that should give us a good idea of common needs. Mobile particularly seems like it should force us down the road of lighter requests.

rjmackay · 2017-05-15T02:06:47Z

My main concern looking at the Repos is that our data model needs to be restructured specifically for forms, stages, attributes and posts. If nothing else we should take the chance to rename them to match the client. That is probably true for Tags as well.

Agreed. I'm trying to just get rid of Kohana first, without rewriting the world in the process. So I'll get through the bulk of this, and the data provider refactor before I tackle the data model shift...

We'll have to come up with a plan for managing old clients if we shift the data model.

willdoran · 2017-05-12T14:59:08Z

app/Http/Controllers/API/Posts/PostsController.php

+		];
+	}
+
+	protected function getPayload(Request $request)


Will this eventually be redundant in relation to the one getFilters(or vice versa) or is it just a place holder for now?

No. Filters are built from the query string, payload is built from the JSON body of a request.

willdoran · 2017-05-12T15:00:47Z

app/Http/Controllers/API/Posts/PostsController.php

+	 *
+	 * @return void
+	 */
+    public function store(Request $request)


It's nice to have clear verbs

willdoran · 2017-05-12T15:01:04Z

app/Http/Controllers/API/Posts/RevisionsController.php

willdoran · 2017-05-12T15:04:24Z

app/Http/Controllers/RESTController.php

+     * @return string
+     * @todo  move this somewhere sensible
+     */
+    public static function url($resource, $id = null)


I feel like this is part of a Util class - which I think should usually be called a misc class. Would it be better as a service class?

Yup. See the @todo move this somewhere sensible :)

willdoran · 2017-05-12T15:06:20Z

app/Http/Controllers/RESTController.php

+            }
+
+            // Should we prevent this request from being cached?
+            if (! in_array($request->method(), $this->cacheableMethods)) {


This seems very manual, should cache control not belong to the web server?

This is straight from the Kohana version, but in short no. The web server knows nothing about the content its serving, so it doesn't know if its safe to cache or not.
I think originally this was used to make sure GET responses were cachable, but POST/PUT/DELETE were not. I suspect its not actually working now

willdoran · 2017-05-12T15:08:34Z

app/Providers/AuthServiceProvider.php

+        Passport::loadKeysFrom(storage_path('passport/'));
+        // Define possible scopes
+        // @todo simplify / improve these
+        Passport::tokensCan([


I like that these come with descriptions

willdoran · 2017-05-12T15:11:04Z

tests/integration/oauth.feature

@@ -45,7 +45,7 @@ Feature: Testing OAuth2 endpoints
    Scenario: Authorized Posts Request
        Given that I want to update a "Post"
        And that its "id" is "95"
-        And that the request "Authorization" header is "Bearer testingtoken"
+        And that the oauth token is "testingtoken"


I like the language simplifications

Mostly due to having to generate JWT tokens in RestContext grr.

rjmackay added the Stage: In Review label Apr 6, 2017

rjmackay force-pushed the lumen-controllers branch 2 times, most recently from 6050834 to cdb48fe Compare April 6, 2017 01:22

rjmackay force-pushed the lumen branch from 6770944 to e746cac Compare April 13, 2017 05:44

rjmackay force-pushed the lumen-controllers branch 2 times, most recently from fcc76b7 to aa96ca9 Compare April 19, 2017 01:51

rjmackay force-pushed the lumen-controllers branch 2 times, most recently from 4f22036 to 4bf6300 Compare April 21, 2017 04:37

rjmackay requested a review from willdoran April 24, 2017 02:32

rjmackay force-pushed the lumen-controllers branch from 282b36a to a0cd168 Compare April 26, 2017 08:30

rjmackay force-pushed the lumen branch from e746cac to f7e48d5 Compare May 1, 2017 22:51

rjmackay force-pushed the lumen-controllers branch 4 times, most recently from 0248d2f to 9c197a8 Compare May 3, 2017 02:20

rjmackay mentioned this pull request May 3, 2017

Migrate from kohana to laravel / lumen #1372

Closed

rjmackay force-pushed the lumen branch from f7e48d5 to 8cdd1fc Compare May 11, 2017 02:20

rjmackay added 13 commits May 11, 2017 14:36

Migrate REST controller to lumen

5dae71d

- Create base REST Controller w/ pre + post processing - Call usecases - Make errors JSON if the request asks for JSON - Minor fixes to REST controller - Add CORS Middleware to set headers - Add config controller to test with

Begin adding laravel/passport + lumen-passport

b7e4448

- Hook up auth to our UserRepo - Add migrations for passport - Connect Passport to Ushahidi Users so that auth actually works!

Fix up oauth handling in tests to generate JWT tokens

e267730

Fix response header checks in behat tests

485e9b1

Update collection formatter to use lumen helpes

5543ab4

Get Tags and Config API working

69e2b73

Add Collections & Contacts controller, fix tests

efcf364

Direction httpdocs over to public/

b524475

Update tests to use new oauth token step

e6b1778

Add dataproviders controller

ac1f360

getScope -> getResource

5c463e4

Clean up config controller

032dc9f

Add forms, stages, attributes and roles API

b955822

rjmackay added 19 commits May 11, 2017 14:38

Add migrate controller

fd7dfd6

Add posts API controllers

8216e83

- Add posts, revisions, translations APIs - Add export and geojson api - Fix up RestContext issues in testing - Excluding updates API - Still need to fix up validation error output

Format validation errors to jsonapi-ish format (same as pre lumen)

a3f0210

Remove the concept of post updates since they're unused

1e13fab

Add collections/posts API controller

06e39b2

Add CSV Import controllers

e73ed27

Fix posts stats route

49bee06

ACL fixes for updates being removed

d9600f2

Handle error response special cases

fb0d013

Fix ACL tests

64a0347

Fix oauth errors

a530836

- Make sure we're outputting JSON errors - No longer accepting access token via query param - Return 401 not 400 for missing access token - Return 401 not 400 for incorrect password

Fix up testing environments

070cc93

Rename RestController to RESTController

6f2047b

Run composer pre-test in codeship test build

bcafbc8

Fix lumen APP_KEY values in testing

b3c9959

Don't reference Ushahidi_Rest controller from formatters

06b650f

Delete old Kohana Controllers

32e0665

Remove unused UshahidiUserProvider

d93d000

Fix all the linting

87e62f1

rjmackay force-pushed the lumen-controllers branch from 3ea2ab4 to 87e62f1 Compare May 11, 2017 02:38

Add laravel-cors package and use that to handle CORS responses

f7948ef

- Remove custom CORS middleware - Add tests for preflight requests - Add tests for CORS requests - Add tests for vanilla OPTIONS requests - Add routes for /config and /posts OPTIONS requests

rjmackay changed the title ~~WIP: Migrate REST controller to lumen~~ Migrate API controllers to lumen May 11, 2017

rjmackay merged commit f88979f into lumen May 15, 2017

rjmackay removed the Stage: In Review label May 15, 2017

willdoran reviewed May 15, 2017

View reviewed changes

rjmackay mentioned this pull request May 16, 2017

Migrate to Lumen #1663

Merged

13 tasks

rjmackay deleted the lumen-controllers branch May 9, 2018 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate API controllers to lumen #1674

Migrate API controllers to lumen #1674

rjmackay commented Apr 6, 2017 •

edited

Loading

rjmackay commented May 12, 2017

willdoran commented May 12, 2017

rjmackay commented May 15, 2017

willdoran May 12, 2017

rjmackay May 15, 2017

willdoran May 12, 2017

willdoran May 12, 2017

willdoran May 12, 2017

rjmackay May 15, 2017

willdoran May 12, 2017

rjmackay May 15, 2017

willdoran May 12, 2017

willdoran May 12, 2017

rjmackay May 15, 2017

Migrate API controllers to lumen #1674

Migrate API controllers to lumen #1674

Conversation

rjmackay commented Apr 6, 2017 • edited Loading

rjmackay commented May 12, 2017

willdoran commented May 12, 2017

rjmackay commented May 15, 2017

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

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

Choose a reason for hiding this comment

rjmackay commented Apr 6, 2017 •

edited

Loading