THREESCALE-12410: Remove :api signup type#4293
THREESCALE-12410: Remove :api signup type#4293jlledom wants to merge 6 commits into3scale:masterfrom
:api signup type#4293Conversation
It's redundant, completely overlapped by :created_by_provider
|
|
||
| user.unflattened_attributes = flat_params | ||
| user.signup_type = :api | ||
| user.signup_type = :created_by_provider |
There was a problem hiding this comment.
Users created via API will have the :created_by_provider type from now on. However, we still have about 50k users in the DB with the :api type, and I don't plan to migrate them in any way. See other my other comments to see how I handled this.
| def api_signup? | ||
| signup.api? | ||
| end | ||
|
|
There was a problem hiding this comment.
This, and api? are not called from anywhere now. Can can cleanup them.
|
|
||
| %i[minimal api created_by_provider].each do |type| | ||
| user.signup_type = type | ||
| assert_not user.signup.by_user?, "expected by_user? to be false for :#{type}" |
There was a problem hiding this comment.
I hate these AI explanations in assertions, the text adds nothing to the standard error message. Not mandatory to do anything but I wanted to vent out.
There was a problem hiding this comment.
I consider rage against AI very healthy so I approve your comment
Edit: a8b8cda
|
Shouldn't we add the notification to the wrt keeping |
It should behave exactly as before
Not directly related to this PR, but it's good to have these tests to ensure no regressions happen The expected behavior is: when creating a partner user, we must provide either a pass or an open_id, but at least one.
|
|
||
| def by_user? | ||
| not machine? | ||
| (new? || signup_type.nil? || partner?) && !open_id? && !cas? && !oauth2? |
There was a problem hiding this comment.
This code is meant to implement backwards compatibility without mentioning :created_by_provider or :api types. It should behave exactly as before.
:new_signup(they signed up themselves) -> human:nil(accepted invitation) -> human:partner-> human
In all cases, if they have SSO attributes, they are considered machine.
| test 'post without password or open_id rejected' do | ||
| prepare_master_account | ||
| post :create, params: provider_params.except(:open_id) | ||
|
|
||
| assert_response :unprocessable_entity | ||
| body = JSON.parse(response.body) | ||
|
|
||
| refute body['success'] | ||
| assert body['errors']['user']['password'].present? | ||
| end | ||
|
|
There was a problem hiding this comment.
The tests about partner users are not directly related to the changes, but they ensure no regressions are caused. We need them.
I don't think we should send notifications because both
I don't like to touch the DB unless really needed, migrations are risky and make deploying/updating more complicated. The code is backwards compatible, I think it's enough. Also, I still don't know which changes will I make in order to get rid of more signup types, or if I'll be able to remove them all. If we consider a migration seriously, better do it after I have investigated all signup types and decided what to do with them. |
My line of thinking was that once an admin created a user, the user should benotified that they have an account. Like one provider but for example I am a 3scale admin and create an account for you. You want to be notified as soon as possible to unsubscribe and remove your account.
Makes sense. |
2 participants
What this PR does / why we need it:
We'd like to remove as many signup types as possible since they complicate all the signup logic and they are created to implement some use cases that might be solved in some other way.
This PR removes the
:apisigup type. This one is only set by the controllers:app/controllers/admin/api/buyers_users_controller.rb:20 — buyer users created via APIapp/controllers/admin/api/users_controller.rb:22— provider users created via APIAnd only read from
User::SignupType#by_user?to determine whether the signup was by human or by machine.I think this signup type is redundant, because is completely overlapped by
:created_by_provider. See the table at the jJira issue:So this PR removes the
:apitype and assigns to the users created via those endpoints the:created_by_providertype.Breaking changes
:created_by_provider, socreated_by_provider_signup?returns true, and the approval notification email is suppressed.Previously with :api, it was not suppressed. This is the one behavioral change we identified earlier.
:created_by_providerinstead of:apiin analytics. Not a code breakage, but a data change.Everything else should behave exactly like it does now, AFAIK.
Which issue(s) this PR fixes
This belongs to https://redhat.atlassian.net/browse/THREESCALE-12410 but it doesn't fix it completely, the logic is so complex I decided to do it type by type, one PR each.
Verification steps
Test should pass