THREESCALE-12410: Remove `:api` signup type by jlledom · Pull Request #4293 · 3scale/porta

jlledom · 2026-04-30T10:18:13Z

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 :api sigup type. This one is only set by the controllers:

app/controllers/admin/api/buyers_users_controller.rb:20 — buyer users created via API
app/controllers/admin/api/users_controller.rb:22 — provider users created via API

And 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 :api type and assigns to the users created via those endpoints the :created_by_provider type.

Breaking changes

Approval email suppression (account/states.rb:178): New API-created buyer users, for buyers that require approval, received the approval email notification before. Now get signup_type = :created_by_provider, so created_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.
Analytics tracking (user_tracking.rb:95): New API-created users will report signup_type = :created_by_provider instead of :api in 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

It's redundant, completely overlapped by :created_by_provider

jlledom · 2026-04-30T10:21:35Z


    user.unflattened_attributes = flat_params
-    user.signup_type = :api
+    user.signup_type = :created_by_provider


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.

jlledom · 2026-04-30T10:22:17Z

-  def api_signup?
-    signup.api?
-  end
-


This, and api? are not called from anywhere now. Can can cleanup them.

akostadinov · 2026-04-30T10:49:50Z

+
+    %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}"


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.

I consider rage against AI very healthy so I approve your comment

Edit: a8b8cda

akostadinov · 2026-04-30T10:51:27Z

Shouldn't we add the notification to the created_by_provider users?

wrt keeping :api, I prefer to have a migration to update that value. It is very frustrating to hit unexpected data in the database. But also not super hard opinion.

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.

jlledom · 2026-04-30T12:41:53Z


    def by_user?
-      not machine?
+      (new? || signup_type.nil? || partner?) && !open_id? && !cas? && !oauth2?


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.

Hm.... I don't really like this solution too much...
Not only I find it a bit confusing (hard to read), but also I think it's not the most optimal one.
Previously, machine? method returned as soon as one of the OR conditions was true, now for "human-created" users we'll most likely need to check ALL of the AND conditions, one of which also includes checking for an association (sso_authorizations).

Maybe I'm overthinking, dunno.

But I think, I'd probably just leave the old code, with the following changes:

def machine? minimal? || created_by_provider? || open_id? || cas? || oauth2? end def created_by_provider? %i[created_by_provider api].include?(signup_type) end

i.e. still remove api? check and extend created_by_provider - adding a comment that :api is there for backwards compatibility (also, it kind of reflects that we consider the behavior for both values to be the same)

Fine for me: 17f7f1f

I used that formula to avoid mentioning the :api signup type since it's supposed to not exist anymore, only in the DB. But I'm fine with your suggestion.

jlledom · 2026-04-30T12:42:50Z

+  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
+


The tests about partner users are not directly related to the changes, but they ensure no regressions are caused. We need them.

jlledom · 2026-04-30T13:04:50Z

Shouldn't we add the notification to the created_by_provider users?

I don't think we should send notifications because both :api and :created_by_provider user are, well, created by the provider, who already knows they have created a user. I don't think the notification is needed but don't have a strong opinion. I think it's more important to keep the consistence: do it for both or nothing, since :api and :created_by_provider are essentially the same. That's the behavior from now on, after this PR.

wrt keeping :api, I prefer to have a migration to update that value. It is very frustrating to hit unexpected data in the database. But also not super hard opinion.

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.

akostadinov · 2026-04-30T14:22:55Z

I don't think we should send notifications because both :api and :created_by_provider user are, well, created by the provider, who already knows they have created a user.

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.

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.

Makes sense.

jlledom · 2026-05-05T13:04:54Z

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.

I'm OK with sending the notification. In fact, I think that notification is the only place where the :created_by_provider signup type is used from, so sending it unconditionally would allow us to remove that signup type as well.

Would you agree on merging this and remove the :created_by_provider type in another PR?

akostadinov

It was just my intuition that it makes sense to notify users when it was somebody else creating a user for them.

But we need to make sure some use cases involving generating users with API will not break the email bill.

Either way, nice PR, I trust your assessment for future development.

mayorova · 2026-05-08T16:01:28Z

    assert created_by_provider_user.errors[:password].blank?
  end

+  test 'by_user? returns true only for :new_signup, nil, and :partner signup types' do


should we also test how open_id?, cas? and oauth2? responses affect the by_user? return value?

Here: b006abf

Co-authored-by: Daria Mayorova <mayorova@users.noreply.github.com>

Address code review feedback: keep machine? as the primary definition with explicit OR conditions instead of inverting by_user?. This avoids evaluating all AND conditions (including an association check) for human-created users. Extend created_by_provider? to also match :api for backward compatibility with existing DB records. Co-authored-by: Daria Mayorova <mayorova@users.noreply.github.com>

Add test verifying that SSO identifiers (open_id, sso_authorizations, authentication_id, cas_identifier) override by_user? to return false regardless of signup type. Addresses code review feedback requesting explicit coverage of how SSO flags affect by_user? return value. Co-Authored-By: Claude <noreply@anthropic.com>

akostadinov · 2026-05-12T10:55:39Z

    end
  end

+  test 'by_user? returns false when user has SSO identifiers regardless of signup type' do


I think this should be part of 'by_user? returns true only for :new_signup, nil, and :partner signup types' test. But not a showstopper for me.

I don't mind changing it, but if I do, I'll lose the approval and have to run tests again just for this. I'm gonna wait for @mayorova's comments, if she requests more changes I'll change this as well.

I must finally give you a token with approve privileges so you stop complaining about approvals.

jlledom added 2 commits April 30, 2026 09:46

Remove :api signup type

6d3cc72

It's redundant, completely overlapped by :created_by_provider

Fix/add tests

635135a

jlledom self-assigned this Apr 30, 2026

jlledom commented Apr 30, 2026

View reviewed changes

akostadinov reviewed Apr 30, 2026

View reviewed changes

jlledom marked this pull request as draft April 30, 2026 11:01

jlledom added 2 commits April 30, 2026 14:33

by_user?: consider partner ad SSO scenarios

ed0f179

It should behave exactly as before

Add tests for partners

b577da3

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.

jlledom commented Apr 30, 2026

View reviewed changes

jlledom added 2 commits April 30, 2026 14:44

test: by_user?: consider partners

18d1b81

Rage against the machine

a8b8cda

jlledom marked this pull request as ready for review May 4, 2026 07:50

akostadinov previously approved these changes May 5, 2026

View reviewed changes

mayorova reviewed May 8, 2026

View reviewed changes

Comment thread test/functional/partners/providers_controller_test.rb Outdated

mayorova reviewed May 8, 2026

View reviewed changes

Apply suggestions from code review

8b55462

Co-authored-by: Daria Mayorova <mayorova@users.noreply.github.com>

jlledom dismissed akostadinov’s stale review via 8b55462 May 12, 2026 08:26

jlledom and others added 2 commits May 12, 2026 10:36

jlledom requested review from akostadinov and mayorova May 12, 2026 09:50

akostadinov approved these changes May 12, 2026

View reviewed changes

Conversation

jlledom commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlledom Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akostadinov commented Apr 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlledom commented Apr 30, 2026

Uh oh!

akostadinov commented Apr 30, 2026

Uh oh!

jlledom commented May 5, 2026

Uh oh!

akostadinov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jlledom commented Apr 30, 2026 •

edited

Loading

jlledom Apr 30, 2026 •

edited

Loading

akostadinov left a comment •

edited

Loading