saxifrage / cityasacampus

An open-source platform for connecting and showcasing resources within local learning communities.
http://cityasacampus.org/
5 stars 4 forks source link

implement pathways in Rails #377

Closed chadwhitacre closed 8 years ago

chadwhitacre commented 8 years ago

Server-side for #227. Merge into #364 when ready.

To-do

chadwhitacre commented 8 years ago

Rebased onto create-pathways at 6f22648d365eff9593a2d732ca1d594afce574dd. Previous head was 7366024c66c96a1eca4c7b26d66e6a8c65fbbcb9.

chadwhitacre commented 8 years ago

screen shot 2015-12-11 at 4 46 04 pm

chadwhitacre commented 8 years ago

pf76egfwq8

chadwhitacre commented 8 years ago

Next is to properly associate pathways with grids on save and load.

s0pglu9tlw

chadwhitacre commented 8 years ago

Next week! :-)

chadwhitacre commented 8 years ago

I'm reading https://blog.engineyard.com/2015/active-model-serializers in order to get pathways included in /grids.json.

chadwhitacre commented 8 years ago

Note that I'm using a the edge version of AMS here because it supports belongs_to and other features.

chadwhitacre commented 8 years ago

Eep! Sorry, hit tab once too many times. :)

chadwhitacre commented 8 years ago

Looks like these are the docs for the version of active_model_serializers we're using:

https://github.com/rails-api/active_model_serializers/blob/0.9.3/README.md

chadwhitacre commented 8 years ago

I'm seeing SQL queries for the pathways for a grid, but grid_id is null for all pathways. Let's fix that! :)

chadwhitacre commented 8 years ago

I need CSRF to POST.

chadwhitacre commented 8 years ago

(Why didn't I hit this before?)

chadwhitacre commented 8 years ago

Maybe that's a red herring.

Started POST "/pathways.json" for ::1 at 2015-12-14 10:16:23 -0500
Started POST "/pathways.json" for ::1 at 2015-12-14 10:16:23 -0500
Processing by PathwaysController#create as JSON
Processing by PathwaysController#create as JSON
  Parameters: {"name"=>"cheese", "grid_id"=>9, "pathway"=>{"name"=>"cheese", "grid_id"=>9}}
  Parameters: {"name"=>"cheese", "grid_id"=>9, "pathway"=>{"name"=>"cheese", "grid_id"=>9}}
Can't verify CSRF token authenticity
Can't verify CSRF token authenticity
   (0.2ms)  BEGIN
   (0.2ms)  BEGIN
  SQL (0.9ms)  INSERT INTO "pathways" ("name", "grid_id", "created_at", "updated_at") VALUES ($1, $2, $3, $4) RETURNING "id"  [["name", "cheese"], ["grid_id", 9], ["created_at", "2015-12-14 15:16:23.375967"], ["updated_at", "2015-12-14 15:16:23.375967"]]
  SQL (0.9ms)  INSERT INTO "pathways" ("name", "grid_id", "created_at", "updated_at") VALUES ($1, $2, $3, $4) RETURNING "id"  [["name", "cheese"], ["grid_id", 9], ["created_at", "2015-12-14 15:16:23.375967"], ["updated_at", "2015-12-14 15:16:23.375967"]]
   (0.5ms)  COMMIT
   (0.5ms)  COMMIT
  Grid Load (0.5ms)  SELECT  "grids".* FROM "grids" WHERE "grids"."id" = $1 LIMIT 1  [["id", 9]]
  Grid Load (0.5ms)  SELECT  "grids".* FROM "grids" WHERE "grids"."id" = $1 LIMIT 1  [["id", 9]]
  Pathway Load (0.3ms)  SELECT "pathways".* FROM "pathways" WHERE "pathways"."grid_id" = $1  [["grid_id", 9]]
  Pathway Load (0.3ms)  SELECT "pathways".* FROM "pathways" WHERE "pathways"."grid_id" = $1  [["grid_id", 9]]
Completed 500 Internal Server Error in 26ms (ActiveRecord: 2.4ms)
Completed 500 Internal Server Error in 26ms (ActiveRecord: 2.4ms)

SystemStackError (stack level too deep):
  .bundle/stuff/ruby/2.1.0/gems/actionpack-4.2.1/lib/action_dispatch/middleware/reloader.rb:79
chadwhitacre commented 8 years ago

Yeah, looks like the CSRF protection isn't a hard requirement (though it probably should be), since I'm seeing SQL activity subsequent to it.

chadwhitacre commented 8 years ago

The INSERT succeeds. Where's the 500 from?

chadwhitacre commented 8 years ago

Something to do with the non-uniqueness of the SELECT? There are multiple pathways with grid_id = 9, yet the SELECT has a LIMIT 1.

chadwhitacre commented 8 years ago

Stack level too deep. A nesting problem in the pathways serializer?

screen shot 2015-12-14 at 10 26 55 am

chadwhitacre commented 8 years ago

Got it! The pathways serializer doesn't need to re-reference the grid.

chadwhitacre commented 8 years ago

Next up: linking nodes to pathways!

Then I guess we need rename and remove for grids and pathways.

chadwhitacre commented 8 years ago

Rebased on new #364. Old head was 15bcf2614bb7ba86d402317367cb977361be9444.

chadwhitacre commented 8 years ago

I've almost got grid creation working with a reference to a map (thence to an organizer). I'm missing a bit of wiring to get the organizer_id in the create method of the Organizer controller.

screen shot 2015-12-14 at 6 27 00 pm

Here's the grid_params:

    def grid_params                                                                                           
      params.require(:grid).permit([:name, :organizer_id])                                                    
    end 

Full code in ee05564d3e3f7640656f3ebe0d67a1715c1ca32e. :bug: jumpin' out at you, @dmtroyer?

chadwhitacre commented 8 years ago

Fails in the same way with:

    def grid_params
      params.require(:grid).permit(:name, :organizer_id)
    end
chadwhitacre commented 8 years ago

Is it because organizer_id is not an attribute of the Grid model?

chadwhitacre commented 8 years ago

Alright, gonna call it a day there. :-/

Let me know if you see anything, @dmtroyer. Otherwise I plan to pick up with this on Friday! :-)

dmtroyer commented 8 years ago

@whit537

I've almost got grid creation working with a reference to a map (thence to an organizer). I'm missing a bit of wiring to get the organizer_id in the create method of the Organizer controller.

screen shot 2015-12-14 at 6 27 00 pm

Where's the map reference?

I did notice that there is no longer any reference to organizer on map. It is there in the model, but it was removed from the schema at db/migrate/20151214180556_update_schema_for_maps.rb:8

dmtroyer commented 8 years ago

OK, a big old NM on that last comment. Should have cleaned things up with my last commit.

OH! And @whit537 you will need to run rake db:reset to use the new schema as I went ahead and removed the deletion of organizer_id from the map model in your migration at https://github.com/saxifrage/cityasacampus/commit/d5abf877ff29ba8caa09ec35765aa4044912143d#diff-a8dbbf3c1f23f784a628e0368ddcfdc8L8. Sorry if you had any important data in your development env.

chadwhitacre commented 8 years ago

Where's the map reference?

I did notice that there is no longer any reference to organizer on map. It is there in the model, but it was removed from the schema at db/migrate/20151214180556_update_schema_for_maps.rb:8

Yeah, I was thinking ... I don't remember what I was thinking. <:-)

Sorry if you had any important data in your development env.

Not a problem, I consider dev databases to be ephemeral. :-)

chadwhitacre commented 8 years ago

Rebased on latest #364. Previous head was 1a6b149b20704c5df2c8cefe2ddf2a93b41219ad.

chadwhitacre commented 8 years ago

Alright! Got through the rebasing fun. Now to make some progress ... :camel: :cake:

chadwhitacre commented 8 years ago

What's left on this PR?

chadwhitacre commented 8 years ago

I brought over a good chunk of the to-do from #364 to the description for this PR.

chadwhitacre commented 8 years ago

I'm gonna start a new PR. This one is big enough. Ready for review, @dmtroyer @MatthewVita! :-)

chadwhitacre commented 8 years ago

Should we squash before review, or after review but before merge?

chadwhitacre commented 8 years ago

Personally I find it sometimes helpful to review commit-by-commit to understand why the diff is what it is.

chadwhitacre commented 8 years ago

Let me know if you want me to squash before review, @dmtroyer @MatthewVita. :)

chadwhitacre commented 8 years ago

@dmtroyer Picking up from https://github.com/saxifrage/cityasacampus/pull/384#issuecomment-166403071 ...

I could see making those auth changes here on #377, since this is where we're introducing the new controllers for grids, pathways, and nodes. I used the stock rails generators for those, which I guess doesn't account for authorization (because that comes from a third-party plugin, devise, ya?).

dmtroyer commented 8 years ago

I could see making those auth changes here on #377, since this is where we're introducing the new controllers for grids, pathways, and nodes. I used the stock rails generators for those, which I guess doesn't account for authorization (because that comes from a third-party plugin, devise, ya?).

Works for me. I'm also wondering if we should nest the map, grid and pathway routes to make them more restful.

chadwhitacre commented 8 years ago

Works for me. I'm also wondering if we should nest the map, grid and pathway routes to make them more restful.

Cool. I've added those to the to-do in the description for this PR.

chadwhitacre commented 8 years ago

Alright, let's see about these last couple tasks ...

chadwhitacre commented 8 years ago

configure auth for routes added on this PR

chadwhitacre commented 8 years ago

I'm getting a 404 from http://localhost:3000/api/v1/grids.json with this diff:

diff --git a/client/grids/GridsService.js b/client/grids/GridsService.js
index d551de7..98828a9 100644
--- a/client/grids/GridsService.js
+++ b/client/grids/GridsService.js
@@ -5,14 +5,14 @@ angular.module('caac.grids.service', [
       var logger = $log.getInstance('GridsService');

       var selectAll = function(callback) {
-        return $http.get('grids.json');
+        return $http.get('/api/v1/grids.json');
       };

       var insert = function(organizer_id, name) {
         logger.info( 'attempting to insert a new grid named "' + name
                    + '" for organizer ' + organizer_id
                     );
-        return $http.post('/grids.json', {name: name, organizer_id: organizer_id});
+        return $http.post('/api/v1/grids.json', {name: name, organizer_id: organizer_id});
       };

       return {
diff --git a/config/routes.rb b/config/routes.rb
index 3508487..c9fb686 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -22,7 +22,6 @@ Rails.application.routes.draw do
     resources :opportunity_instances
   end

-  resources :grids
   resources :pathways
   resources :nodes
   resources :topics, except: [:edit, :new]
@@ -45,6 +44,8 @@ Rails.application.routes.draw do
       mount_devise_token_auth_for 'User', at: 'auth'

       get '/organizers/for-current-user.json', to: 'organizers#for_current_user'
+
+      resources :grids
     end
   end
 end
chadwhitacre commented 8 years ago

Looks like the controller is supposed to be nested to suit?

screen shot 2015-12-28 at 10 30 50 am

screen shot 2015-12-28 at 10 30 29 am

chadwhitacre commented 8 years ago

Got it.

chadwhitacre commented 8 years ago

Alright, @dmtroyer, auth for routes configured in 7447d0f2847411b4dd5213e23290c778c840ff19. Look okay? I think we should skip nesting URLs in the interest of time, which means this PR is ready for review and merge!

chadwhitacre commented 8 years ago

Hmm ... we need to constrain the listing of grids by the authenticated user. That could probably be accomplished with nesting ...

chadwhitacre commented 8 years ago

I've got grids filtered by user, but users can see each others ...

chadwhitacre commented 8 years ago

Alright, fixed in 5cc933e06bacc74bb702b8d88eeeceba178c8dea.

chadwhitacre commented 8 years ago

Gonna rebase and squash ...

chadwhitacre commented 8 years ago

Rebased onto e4630c337f74cd9736d8a1420f56ec2523765fb2 and sqaushed. Previous head was e5b3ea6a13351c77deb02e130734cbed02956a3e.