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

Add ability to customize raise_user_error_list method #262

Closed
vitaliiorlov opened this issue Oct 24, 2023 · 11 comments · Fixed by #263
Closed

Add ability to customize raise_user_error_list method #262

vitaliiorlov opened this issue Oct 24, 2023 · 11 comments · Fixed by #263
Labels
enhancement New feature or request

Comments

@vitaliiorlov
Copy link

vitaliiorlov commented Oct 24, 2023

What is the problem the enhancement will solve?

Our project has a strict structure of errors with custom GraphQL error classes.
And there is no possibility to customize the error structure in this gem.

Describe the solution you have in mind

You raise this error like this:

def raise_user_error_list(message, errors:)
  raise DetailedUserError.new(message, errors: errors)
end

raise_user_error_list(
  I18n.t('graphql_devise.passwords.update_password_error'),
  errors: resource.errors.full_messages
)

It would be great to have full control of this process. I would like to specify my own DetailedUserError class for this and also not to pass resource.errors.full_messages as an argument, I need a more detailed object, for example at least resource.errors or better you could change the logic in such way I would have to define some proc which will be passed to the raise_user_error_list method:

def raise_user_error_list(message, errors:)
  error_klass_name = gem_config.detailed_user_error_class_name.presence || "DetailedUserError"
  error_obj = errors.is_a?(Proc) ? instance_exec(errors) : errors # `instance_exec(errors)` to execute the Proc in the current context or `errors.call` if it's a Proc

  raise error_klass_name.constantize.new(message, errors: error_obj)
end

I hope you understand the issue and looking forward to your answer and hope this suggestion will be implemented.
Thanks, guys for your awesome work!

@mcelicalderon mcelicalderon added the enhancement New feature or request label Oct 24, 2023
@mcelicalderon
Copy link
Member

Thank you for your report, @vitaliiorlov! We'll take a look into this shortly. As usual, you are welcome to propose a change if you had a particular implementation in place before we get a chance to look into this :) as long as it's generic and users can opt-in to this new behavior rather than having a breaking change

@vitaliiorlov
Copy link
Author

@mcelicalderon thanks for your reply. Currently, I don't have any implementation of this. If I will, I will create a PR.

@mcelicalderon
Copy link
Member

Taking a quick look, I think we should provide a way for users to specify a base mutation from which all mutation in this gem will inherit from. That way you could override the methods that raise errors in that base mutation class that you provide. This will also allow other type of customizations like setting argument classes and other things you can do at the mutation level

@mcelicalderon
Copy link
Member

@vitaliiorlov are you mounting this gem in your existing schema or are you using the separate route approach?

@vitaliiorlov
Copy link
Author

@mcelicalderon sounds good. But it won't solve my problem at all because you are already passing record.errors.full_messages to the raise_user_error_list method. If you could change this behavior by passing record.errors to the raise_user_error_list and then inside the method call errors.full_messages then it could solve my problem if I override your raise_user_error_list method in my base mutation class if you allow this as you mentioned above.

@vitaliiorlov are you mounting this gem in your existing schema or are you using the separate route approach?

I mount it in the existing schema.

@mcelicalderon
Copy link
Member

If you could change this behavior by passing record.errors to the raise_user_error_list and then inside the method call errors.full_messages then it could solve my problem

That makes sense. We can do that too

@mcelicalderon
Copy link
Member

@vitaliiorlov I have opened #263 which I think is enough to accomplish what you are trying to do, but let me know if it doesn't.

With that simple change, you can write your own mutation that implements a different raise_user_error_list(message, resource: resource). So something like this:

  1. Write your own custom mutation that inherits the one you are interested in from this gem. We do the same on our specs in https://github.com/graphql-devise/graphql_devise/blob/b1435baabc1356fcc2c463a2b1f37a38e89abaae/spec/dummy/app/graphql/mutations/register_confirmed_user.rb
  2. Then, when using the plugin in your existing schema, you can specify this new mutation in your schema loader as specified in the docs https://github.com/graphql-devise/graphql_devise#available-mount-options using the operations option.

Define a custom mutation with your custom method definition

# app/graphql/mutations/custom_register.rb
module Mutation
  class CustomRegister < GraphqlDevise::Mutations::Register
    private
    
    def raise_user_error_list(message, resource:)
      # your custom behavior, resource will be an active record instance if you are using active record
    end
  end
end

specify the mutation for the operation you wish to change

class AppSchema < GraphQL::Schema
  use GraphqlDevise::SchemaPlugin.new(
    query:                Types::QueryType,
    mutation:             Types::MutationType,
    resource_loaders:     [
      GraphqlDevise::ResourceLoader.new(
        User,
        only: [:register],
        operations: {
          register: Mutations::CustomRegister
        },
      )
    ]
  )

  mutation(Types::MutationType)
  query(Types::QueryType)
end

Let me know if this works for you and I can release a new version of the gem that includes #263 so you can do what I have described here.

I think that doing something like what I described in #262 (comment) might be unnecessary if we can already do something similar by specifying the actual mutation and not a base mutation that all other will inherit from.

@vitaliiorlov
Copy link
Author

vitaliiorlov commented Oct 30, 2023

Hey @mcelicalderon!
I've just tried your approach and yeah, it resolved my issue, now I can render errors in my format.
But I noticed new issue which appears when I enabled custom operation in schema as you exampled above.
Field authenticatable disappearing from userUpdatePasswordWithToken mutation.

image

When I am disabling custom operation, it is appearing again.

image

@mcelicalderon
Copy link
Member

@vitaliiorlov yes, this behavior is expected. Whenever you specify a custom mutation, we do not make any changes to it compared to when you don't specify one and we set the authenticatable field automatically. For you use case, please simply add the missing field to your custom mutation. For reference, this is how the gem sets that field

mutation.field(:authenticatable, @authenticatable_type, null: false)

@vitaliiorlov
Copy link
Author

@mcelicalderon got it. Waiting for merging. Thanks a lot!

@mcelicalderon
Copy link
Member

mcelicalderon commented Oct 30, 2023

No problem, @vitaliiorlov! v1.4.0 has been released with this change

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 a pull request may close this issue.

2 participants