zooniverse / talk-api

Apache License 2.0
6 stars 0 forks source link

build service before validating #328

Closed Tooyosi closed 4 months ago

Tooyosi commented 4 months ago

Issue happens cos service.rooted_params is an object that matches the :create_params here: https://github.com/zooniverse/talk-api/blob/4347b0ede6adc4cb8f00700c788ed6c5ccf897af/spec/services/discussion_service_spec.rb#L7

However, the validate class receives a different object as rooted_params during validation here:https://github.com/zooniverse/talk-api/blob/4347b0ede6adc4cb8f00700c788ed6c5ccf897af/app/services/concerns/talk_service.rb#L68

The new object has a user_id parameter added to both the discussion object and the comment array of objects and that makes it different from the :create_params.

I traced down to set_user: https://github.com/zooniverse/talk-api/blob/4347b0ede6adc4cb8f00700c788ed6c5ccf897af/app/services/concerns/talk_service.rb#L86 and noticed unrooted_params has a user_idset to it and this impacts permitted_params aswell (my guess here is cos we're passing permitted_params directly from unrooted_params as opposed to a clone of it)

Making unrooted_params use a clone of permitted_params works fine but increases failing tests across and i added the user_id directly to the :create_params mentioned above, this works but not totally convinced. The 3rd solution is to build the service which is on the pr. What do you think?

yuenmichelle1 commented 4 months ago

Apologies for the delay. I wanted to figure out why this passed in Rails 4.2 but not in Rails 5.0. As far as I can tell, behavior acts the same in Rails 4.2 and Rails 5.0 (in that

  1. rooted_params initially is the params set in create_params
  2. and once we hit TalkService#validate
  3. TalkService#validate calls the action (in this case :create) which builds and sets the user when the local schema gets set in TalkService#validate.
  4. then rooted_params now has the extra user_id attr. Again this ^ behavior happens in both 4.2 and 5.0)

I believe the only change in behavior here is now the tests in Rails 4.2 and Rails 5.0 have different "snapshots" of service.rooted_params. Which makes sense since we are upgrading rspec-rails to a higher version, so there is probably different test behavior associated with it.

This change looks good to me