Skip to content

Set base controller from route mount#237

Merged
00dav00 merged 3 commits intomasterfrom
set_base_controller_from_route_mount
Oct 31, 2022
Merged

Set base controller from route mount#237
00dav00 merged 3 commits intomasterfrom
set_base_controller_from_route_mount

Conversation

@00dav00
Copy link
Copy Markdown
Contributor

@00dav00 00dav00 commented Aug 20, 2022

This PR adds the option base_controller on the mount method. This will create a controller on the fly using the specified controller as the parent.

@00dav00 00dav00 marked this pull request as draft August 20, 2022 06:51
Copy link
Copy Markdown
Member

@mcelicalderon mcelicalderon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments about an alternative approach, let me know what you think. I think it should work

else
Class.new(ActionController::Base)
end
ApplicationController = Class.new(GraphqlDevise.base_controller)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this fail in production with eager load? I think this would happen before the mount method gets executed during app start.

Comment thread lib/graphql_devise/route_mounter.rb Outdated
module GraphqlDevise
module RouteMounter
def mount_graphql_devise_for(resource, options = {})
GraphqlDevise.setup_base_controller(options.delete(:base_controller))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we build a new controller class every time the method is called? Maybe something like:

# do not execute if base_controller option is not present
new_controller = GraphqlDevise.const_set("#{resource}AuthController", Class.new(options.delete(:base_controller)))

new_controller.include(SetUserByToken)
new_controller.include(AuthActionMethod) # module that defines the controller action

post clean_options.at, to: "#{new_controller.to_s.underscore}#auth"
get clean_options.at, to: "#{new_controller.to_s.underscore}#auth"

Not sure that works, but it should?

@00dav00 00dav00 marked this pull request as ready for review October 29, 2022 06:16
Copy link
Copy Markdown
Member

@mcelicalderon mcelicalderon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Left some notes. Also, what do you think about adding a test like the one you introduced for this when using the plugin?

if DeviseTokenAuth.respond_to?(:cookie_enabled)

# frozen_string_literal: true

module GraphqlDevise
module AuthControllerMethods
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about renaming this one to AuthControllerActions?


class CookiesController < ApplicationController
include ActionController::Cookies
protect_from_forgery with: :null_session
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about doing what we did in

instead of setting a null session here? That way we could probably test that the correct cookie is set after login and other mutations

@mcelicalderon mcelicalderon added the enhancement New feature or request label Oct 31, 2022
@00dav00 00dav00 changed the title WIP: Set base controller from route mount (investigation) Set base controller from route mount (investigation) Oct 31, 2022
@00dav00 00dav00 merged commit b4b8721 into master Oct 31, 2022
@00dav00 00dav00 deleted the set_base_controller_from_route_mount branch October 31, 2022 17:51
@mcelicalderon mcelicalderon changed the title Set base controller from route mount (investigation) Set base controller from route mount Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants