Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call log_process_action so that we get status code from Devise controllers #194

Closed
wants to merge 1 commit into from

Conversation

clupprich
Copy link

As described in #67 we're facing an issue when Devise returns a 401 status code (which is set in log_process_action.

With this change we're behaving the same as ActionController::LogSubscriber. I don't really know if this qualifies as a workaround (and what the original intent behind log_process_action is/was).

Please also note that there's an open PR on Devise that moves the setting of the status code from log_process_action to append_info_to_payload.

This way we will get additional payload data (e.g. Devise’s 401
status), and we are consistent with ActionController::LogSubscriber.
@bbuchalter
Copy link

Please also note that there's an open PR on Devise that moves the setting of the status code from log_process_action to append_info_to_payload.

Could you please link to that PR for cross referencing.

@clupprich
Copy link
Author

clupprich commented Jan 10, 2017

@oh, sure, yeah, that would help ;) heartcombo/devise#4375

@bbuchalter
Copy link

Have you tried seeing if the PR in Devise fixes the problem here with no code change?

@benlovell
Copy link
Collaborator

This certainly feels like something better resolved upstream in devise. Can you pursue that PR?

@benlovell
Copy link
Collaborator

This has been resolved upstream.

@benlovell benlovell closed this Nov 22, 2017
@clupprich clupprich deleted the call-log-process-action branch May 4, 2021 20:08
@quentindemetz
Copy link

FYI this was reverted upstream and is still broken as of today with:

  • rails 7.0.3.1
  • lograge 0.12.0
  • devise 4.8.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants