Refs #37936 - Correct success method for jwt api #10412

girijaasoni · 2025-01-09T11:49:22Z

No description provided.

girijaasoni · 2025-01-09T12:27:09Z

Test failures seem unrelated

ofedoren

This controller still weirds me out :/ But if it's agreed upon 🤷, some notes:

Shouldn't this include rather Foreman::Controller::UserAware instead of Foreman::Controller::UsersMixin? There is already logic for self editing check. If it's only for except_hidden, then it's probably worth to either update re-usable place or include only that explicitly.
Why does it have include Foreman::Controller::Parameters::User? It doesn't seem to be used in any way.
It also doesn't need include Foreman::Controller::AutoCompleteSearch, since this concern is for UI controllers.
In API description api :DELETE, '/users/:id/registration_tokens' should rather have :user_id instead of just :id

ofedoren · 2025-01-14T15:27:03Z

app/controllers/api/v2/registration_tokens_controller.rb

@@ -36,7 +36,8 @@ def invalidate_jwt
          raise ::Foreman::Exception.new(N_("No record found for %s"), params[:id])
        end
        @user.jwt_secret&.destroy
-        process_success _("Successfully invalidated registration tokens for %s.\n" % @user.login)
+        login = @user.login
+        render :json => {:status => 'success', :message => _("Successfully invalidated registration tokens for %s." % login), :user => login}, :status => :ok


Suggested change

render :json => {:status => 'success', :message => _("Successfully invalidated registration tokens for %s." % login), :user => login}, :status => :ok

render :json => { :message => _("Successfully invalidated registration tokens for %s." % login), :user => login }, :status => :ok

There is no value in :status => 'success' entry: it's more or less duplicate of the 200 OK status.

Also, having both the success message and :user => login seems like redundance of the information. I'd suggest either not include logins in the message or drop the :user entry.

ofedoren · 2025-01-14T15:27:26Z

app/controllers/api/v2/registration_tokens_controller.rb

@@ -51,7 +52,8 @@ def invalidate_jwt_tokens
        if @users.blank?
          raise ::Foreman::Exception.new(N_("No record found for search '%s'"), params[:search]) end
        JwtSecret.where(user_id: @users).destroy_all
-        process_success _("Successfully invalidated registration tokens for %s.\n" % @users.pluck(:login).to_sentence)
+        login = @users.pluck(:login).to_sentence
+        render :json => {:status => 'success', :message => _("Successfully invalidated registration tokens for %s." % login), :users => login}, :status => :ok


ofedoren · 2025-01-17T12:10:47Z

Please, fix API descriptions and I won't be blocking any longer:

For both endpoints remove dot at the end of descriptions
Instead of :id use :user_id

girijaasoni · 2025-01-17T13:45:03Z

Please, fix API descriptions and I won't be blocking any longer:

For both endpoints remove dot at the end of descriptions

Many places else we're using dots after description. Example. Is there any reason to remove it here

Instead of :id use :user_id

We are mentioning in the description that id means id of the user. I would change it but it means I would also have to change it here and it might break things and I might have to change it at many places. I would prefer not doing that unless absolutely necessary, what do you think?

ofedoren · 2025-01-17T14:04:35Z

Many places else we're using dots after description. Example. Is there any reason to remove it here

I should've mentioned that I meant short description instead of full description:

but it means I would also have to change it here

Yeap. The already stable controllers (https://github.com/theforeman/foreman/blob/develop/app/models/user.rb#L465-L479) can be left untouched, but the new one (https://github.com/theforeman/foreman/blob/develop/app/models/user.rb#L480-L482) is better to follow already established pattern (https://github.com/theforeman/foreman/blob/develop/app/models/user.rb#L474-L479)

and it might break things and I might have to change it at many places.

This feature was added during current development cycle and wasn't released yet. It's just the right time to break and fix stuff.

I would prefer not doing that unless absolutely necessary, what do you think?

I think it's better to polish the new feature than deal with inconsistencies later potentially leaving "as is" since it's API and we always fear to break it piling up tech dept and answer future questions with "well, it was just made like this 🤷 "

girijaasoni · 2025-01-20T22:15:06Z

I think it's better to polish the new feature than deal with inconsistencies later potentially leaving "as is" since it's API and we always fear to break it piling up tech dept and answer future questions with "well, it was just made like this 🤷 "

Thanks @ofedoren , I have addressed and tested everything :)

stejskalleos

🍏 LGTM, but I'll leave final ACK to @ofedoren

ofedoren · 2025-01-21T17:40:33Z

Sorry to bother you once more, but as the last thing: I've added a commit with some smaller fixes.

@girijaasoni, please re-test if it still works as you'd expect.

@stejskalleos, please feel free to merge once is green.

Refs #37936 - Remove dead code, fix not_found message

stejskalleos · 2025-01-22T08:11:53Z

Thanks @girijaasoni @ofedoren

pr-processor bot added Not yet reviewed Waiting on contributor labels Jan 9, 2025

girijaasoni force-pushed the correct-success-jwt branch from 41940ce to 48bd82d Compare January 9, 2025 11:51

pr-processor bot removed the Waiting on contributor label Jan 9, 2025

girijaasoni changed the title ~~Refs 37936 - Correct success method for jwt api~~ Refs #37936 - Correct success method for jwt api Jan 9, 2025

ofedoren reviewed Jan 14, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Jan 14, 2025

girijaasoni force-pushed the correct-success-jwt branch from 48bd82d to d91068e Compare January 17, 2025 10:51

girijaasoni force-pushed the correct-success-jwt branch from d91068e to 0c8fcd0 Compare January 20, 2025 22:13

stejskalleos reviewed Jan 21, 2025

View reviewed changes

ofedoren approved these changes Jan 21, 2025

View reviewed changes

Refs #37936 - Correct success method for jwt api

198316e

Refs #37936 - Remove dead code, fix not_found message

ofedoren force-pushed the correct-success-jwt branch from f9d6292 to 198316e Compare January 21, 2025 18:10

stejskalleos merged commit 1ad2139 into theforeman:develop Jan 22, 2025
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #37936 - Correct success method for jwt api #10412

Refs #37936 - Correct success method for jwt api #10412

girijaasoni commented Jan 9, 2025

girijaasoni commented Jan 9, 2025

ofedoren left a comment

ofedoren Jan 14, 2025

ofedoren Jan 14, 2025

ofedoren commented Jan 17, 2025

girijaasoni commented Jan 17, 2025 •

edited

Loading

ofedoren commented Jan 17, 2025

girijaasoni commented Jan 20, 2025

stejskalleos left a comment

ofedoren commented Jan 21, 2025

stejskalleos commented Jan 22, 2025

	render :json => {:status => 'success', :message => _("Successfully invalidated registration tokens for %s." % login), :user => login}, :status => :ok
	render :json => { :message => _("Successfully invalidated registration tokens for %s." % login), :user => login }, :status => :ok

Refs #37936 - Correct success method for jwt api #10412

Refs #37936 - Correct success method for jwt api #10412

Conversation

girijaasoni commented Jan 9, 2025

girijaasoni commented Jan 9, 2025

ofedoren left a comment

Choose a reason for hiding this comment

ofedoren Jan 14, 2025

Choose a reason for hiding this comment

ofedoren Jan 14, 2025

Choose a reason for hiding this comment

ofedoren commented Jan 17, 2025

girijaasoni commented Jan 17, 2025 • edited Loading

ofedoren commented Jan 17, 2025

girijaasoni commented Jan 20, 2025

stejskalleos left a comment

Choose a reason for hiding this comment

ofedoren commented Jan 21, 2025

stejskalleos commented Jan 22, 2025

girijaasoni commented Jan 17, 2025 •

edited

Loading