So, this is different from other API controller, which have only plain parameters, i.e. not nested under extra_fields.
I only did this because there was an existing test in test/integration/admin/api/accounts_controller_test.rb which was passing extra params in this way:

params: update_params.merge(extra_fields: { my_field: 4 })

I also added a check that plain parameters would also work.

Now I'm not sure if other API controllers need to accept both ways too 🤔

This was indeed added explicitly in this PR: https://github.com/3scale/porta/pull/3215/changes

However, later the implementation of the spam check was changed, and this method became dead code: a798bc0

feels like all these are redundant in the presence of test_provider_fields

Well, I think the tests are not redundant, because each one tests a different method.

However, now I'm thinking - why didn't I use defined_fields_names_for everywhere instead of the alternatives?..

I guess the reason is that at first it hadn't occurred to me. I was relying on the existing defined_fields_names and similar, whose implementation is actually more complex, because it:

1. requires a properly initialized instance - which is a bit inconvenient, and made me build the new instance of objects with proper associations before being able to derive the list of defined fields
2. includes calculating the fields_definitions_source_root etc.

It makes me think - should I refactor the whole PR to use this simpler defined_fields_names_for(class_name)? 🤔

overlooked the actual method names, too late when I was looking at these :)

should I refactor the whole PR to use this simpler defined_fields_names_for(class_name)?

If not too intrusive, it would be great but can go in another PR as well.

I assume we have some cucumberf for this. What are we testing? nothing_raised 😅 ?

Pretty much 😉

I think it doesn't hurt, and it's good practice to test every action of the controller.
Now, could these tests be more extensive (e.g. assert something more) - of course. But I think it's better to have a status check than not have the test at all. I'll try to add a couple of more assertions (e.g. that the drop is assigned).

Bob said that previously signup_type: :new_signup was set by service while now it is set to minimal. Is this a valid concern?

just a nit, this is much better 2 tests

the removed comment contains useful information that params are required, types, etc. Should we remove it?

After interrogation, Bob thinks that this change is potentially unsafe:

│ Old code (SAFE):                                                             │
│  1 @plan_id ||= params.require(:cinstance).permit(:plan_id).tap { |          │
│    plan_params| plan_params.require(:plan_id) }[:plan_id]                    │
│  - .permit(:plan_id) filters out arrays/hashes, only allowing scalar         │
│    values                                                                    │
│  - Result with plan_id: [1,2,3] → returns empty {}, then [:plan_id]          │
│    returns nil                                                               │
│                                                                              │
│ New code (POTENTIALLY UNSAFE):                                               │
│  1 @plan_id ||= cinstance_params.require(:plan_id)                           │
│  - require(:plan_id) does NOT filter arrays - it just checks the key         │
│    exists                                                                    │
│  - Result with plan_id: [1,2,3] → returns the array [1, 2, 3]      

Maybe not really, because plan_id used in #find shouldn't cause real troubles but still better to make sure we get a proper value or no value.

If we stopped using application_attrs as I see in app/controllers/api/applications_controller.rb and app/controllers/buyers/applications_controller.rb, why don't we remove it altogether? Since the method signature changed so much, there shouldn't be any hidden users that we would need backward compatibility for. As well it will be harder to remove this later as one will suspect existing users.