feat: Add GET route for sign out on static error pages #5189
Conversation
|
LGTM functionally. over to @dorner for technical review, though I do note that the linter failed |
Could it be because my branch is not up to date with main? |
|
Maybe? I think I've seen some folk talking about some rubocop weirdness since the update to 3.4 on the slack. |
|
Sorry @Gabe-Torres -- I thought you were going to do the merge of main into your branch! Have done it, and the linter is now passing. @dorner -- Can you do the technical pass on this, please? Thanks! |
I thought we were waiting for the code approval before the merge of main. Sorry about that! Thanks @cielf ! |
|
@Gabe-Torres We wait for code approval for merge of your branch into main. We can merge the current main into your branch whenever. |
- Add GET route for sign out in devise_scope block in routes - Add sign_out action to sessions controller - Add feature tests for logout on error pages - Remove data-method="delete" from error pages
- Rename sign_out method to sign_out_error_page in sessions controller - Update route to use new method name
Gabe-Torres
force-pushed
the
5101_static_error_pages
branch
from
b68702d to
a7522b6
Compare
|
I think this is in @dorner's hands for review later this week. |
| end | ||
|
|
||
| # GET /resource/sign_out | ||
| def sign_out_error_page |
There was a problem hiding this comment.
I'm a bit confused... doesn't Devise already generate a sign_out route? Is there a reason you can't reuse that?
There was a problem hiding this comment.
The reason for the custom route is that the static error pages (403, 404, 422, 500) are served as plain HTML files and can only make GET requests. Devise's standard sign-out route requires a DELETE request. I believe another way to reuse Devise's included sign-out is to make JavaScript scripts
There was a problem hiding this comment.
Yes, I agree and confirm -- upsettingly devise does use the DELETE which is actually sent as a POST with a method. The other way to do this would be to make them form submits, but that seems wrong also. I like it this way as a new endpoint.
|
@Gabe-Torres -- Are you still working on this? |
|
Sorry for the super delayed response y'all! I recently started a new position and got a bit busy |
|
... buuuut the tests fail :) |
|
Ok PIVOT! Let's switch to using a form-button for the logout so that it can send a POST+DELETE without needing javascript. Let me know if that makes sense or not. |
4 participants
Resolves #5101
Description
Adds the ability for users to sign out from error pages (403, 404, 422, 500)
Type of change
How Has This Been Tested?
log in as org_admin1@example.com
then go to http://localhost:3000/500
click on "Log out"
Screenshots
404 error page
422 error page
403 error page
500 error page