bullet-train-pro / bullet_train-action_models

Other
5 stars 1 forks source link

Add support for new API #19

Closed gazayas closed 1 year ago

gazayas commented 2 years ago

Closes #18. (Adding a WIP tag since draft pull requests can't be opened on this PR yet)

Changes

I decided to start with the targets-many action. What I've done with this first commit is

  1. Add a new API controller with a StrongParameters module at the bottom of it.
  2. Call include strong_parameters_from_api from the original controller.
  3. Ensure the API controller is ejected to the main application when running the Super Scaffold command.

Testing, Routing, and Jbuilder Templates

Before moving onto the other actions, I want to make sure I have the routes in place and that we're handling the Jbuilder templates correctly by adding controller tests. Looking over this repository, it looks like we don't have any tests in place for the controllers in general, unless I'm overlooking something. Since the new API brought in some new breaking changes, I want to make sure that that's pretty firmly settled before trying to tackle all the other controllers.

We DO have API controller tests for normal Super Scaffolded crud models though, so I think we can start there and try to adjust them to fit the action models.

gazayas commented 2 years ago

We have an API controller for TangibleThing out of the box on the starter repository, but since there are no TangibleThing Action Models initially set up, I think it would be good to write some tests here that we would Super Scaffold to the starter repository, instead of just having a stock API test over there like we do with the TangibleThing API controller test.

Since we have Jbuilder templates when Super Scaffolding we won't have to worry about that, the only thing is routing and the tests themselves.

I'm working on the tests, but haven't quite gotten them to work yet. I want to make the following work, and then traverse the process and write it as a TangibleThing API test in this repository so we get the proper Super Scaffolded file.

rails g model Project team:references name:string
bin/super-scaffold crud Project Team name:text_field
bin/super-scaffold action-model:targets-many Archive Project Team

The test looks like this

require "controllers/api/v1/test"

if defined?(BulletTrain::ActionModels)
  class Api::V1::Projects::ArchiveActionsControllerTest < Api::Test
    def setup
      super

      @test_user = create(:onboarded_user)
      @projects = create_list(:project, 3, team: @test_user.current_team)
      @projects_archive_action = create(:projects_archive_action, team: @test_user.current_team, created_by: @test_user.memberships.first, target_ids: [@projects.first.id.to_s])

      # This should eventually run automatically when creating a new action.
      @projects_archive_action.perform_on_target(@projects.first)
    end

    test "show" do
      get "/api/v1/projects/archive_actions/#{@projects_archive_action.id}", params: {access_token: access_token}
      assert_response :success
    end

    test "index" do
      get "/api/v1/teams/#{@test_user.current_team.id}/projects/archive_actions", params: {access_token: access_token}
      assert_response :success
    end
  end
end

In both instances I'm getting a 404.

Anyways, that's the direction I'm going in with this, I think as long as the tests are figured out the rest shouldn't be too bad.

gazayas commented 2 years ago

Ok, made some progress. The Jbuilder templates needed to be under the app/views/api/v1/ folder, and I had to edit the partial name for archive_actions.

I'm thinking we'll have to move the files to that directory AFTER scaffolding all of the files because we create a new directory with the name of the parent.

gazayas commented 2 years ago

@andrewculver I plan on doing the same for targets-one and targets-one-parent, but since targets-many is ready I've decided to remove the WIP. Instead of pushing everything at once, I thought it would be easier to look at things with just targets-many for now. I'll start working on the other actions in the meantime.

Changes

  1. I removed :delay and :emoji from the Jbuilder template since they don't seem to be present in the scaffolded targets-many model.
  2. I added :created_by_id in the strong params in the API controller since it can't be a null value, which was causing the tests to fail.
  3. In the create test, I used to_json instead of to_api_json on line 63:
    targets_many_actions_data = JSON.parse(build(:completely_concrete_tangible_things_targets_many_action, team: nil, target_ids: [@completely_concrete_tangible_things.first.id.to_s], created_by: @user.memberships.first).to_json)

Routing

I know the routing's still kind of broken, but I went ahead and made the scaffolder add routes with the same logic as the normal routes. To get the tests working in general, I had to edit the routes to look like the example in #14.

No data attribute in JSON response

Other tests have lines like this which dig through a response's data attribute:

assert_proper_object_serialization response.parsed_body.dig("data").first

I found that these responses don't have a data attribute though, so I thought I'd mention it here.

gazayas commented 1 year ago

Since some time has passed on this one, I went ahead and pushed a commit for the targets-one API setup as well. Since the diffs are kind of crazy, I suggest simply super scaffolding each action model one at a time and comparing them with the new code in this pull request.

Some of the changes made for the targets-one action are similar to those mentioned above for the targets-many action.

By the way, to get both targets-many and targets-one to work, I had to manually edit the API routes file to look like the routes in this comment in #14. I might have to open a joint PR in bullet_train-super_scaffolding unless we want to switch over to using the model method in our routes files sooner than later.

One more to go, targets-one-parent.

gazayas commented 1 year ago

I managed to fix the routing! Details in #14.