TAN-2224 Co-sponsors side effects

[IvaKop]

Changelog

[notion-workspace]

💡 Co-sponsorship

[IvaKop]

@kogre Re-creating the PR as the prev one got closed after I deleted the cosponsorship branch. There are still some relevant comment in there you might want to have a look at. To speed things up a bit main question is about the way the co-sposhorship ids diff is being handled and whether this can be done better.

[kogre]

Nice! Some small remarks, but nothing major.

You should also still update the ideas_spec acceptance tests. They should include the new cosponsors_ids in the params list for create and update, maybe also cover the case where we update the consponsors, as the controller plays an important role in that with those cosponsorship ids, so currently there's no coverage of the whole thing not breaking.

I think your solution for detecting the cosponsorships changes is sensible, and robust. The alternative would be to depend on ActiveModel::Dirty, which since Rails 7 also supports associations. When the original initiatives code was written, we weren't on Rails 7 yet, I believe. The advantage of that approach is that it's fully controlled in de side fx, instead of leaking into the controller. That being said, I think this is pretty simple and more robust, so I'm okay with it, don't think it's worth changing.

[cl-dev-bot]

	Warnings
⚠️	The changelog is empty. What should I put in the changelog?

	Messages
📖	Notion issue: TAN-2224
📖	Run the e2e tests
📖	Check translation progress

Generated by 🚫 dangerJS against 8358f9c

[IvaKop]

@kogre I've addressed your comments, thanks for reviewing! I still couldn't figure out how to fix the failing specs.. 🤔
Would be great if you could take a look and let me know what I'm missing 🙌

[kogre]

@IvaKop The reason why the test fails is because the default state of the cosponsor_ids custom_field is set to enabled: false, which means that eventually, it isn't part of the allowed parameters in the controller.

citizenlab/back/lib/participation_method/ideation.rb

Line 325 in 00ad895

enabled: false,

I'm not fully sure what the implcation of the enabled field on the form is wrt to the form builder. Either we should change this boolean, or the failing spec should manually configure the custom field to be enabled.

[IvaKop]

@kogre I find this confusing because this is overridden and set to true when creating a proposal so I'm not sure how to solve it. Let's maybe take a look together.

[kogre]

@IvaKop It's fixed, we had a factory in place to easily create a custom form with the default fields, so I was able to use that, in the end.

[IvaKop]

Thanks @kogre! Clean solution in the end 👍 Really appreciate your help on this one 🙌