Refactor routes mount method by 00dav00 · Pull Request #67 · graphql-devise/graphql_devise

00dav00 · 2020-02-08T13:06:46Z

Refactor routes addition with sanitizer class

Add operation sanititzer class
Add operation preparer class
Refactor routes creation with utilitary classes

mcelicalderon · 2020-02-17T03:13:15Z

+    def call
+      result = {}
+
+      @mutations.each do |action, mutation|


@mutations.each_with_object({}) do |(action, mutation), result| ... result[mapped_action] = new_mutation end

mcelicalderon · 2020-02-17T03:18:48Z

+    def call
+      result = {}
+
+      @queries.each do |action, query|


each_with_object can also be used here

mcelicalderon · 2020-02-17T03:21:27Z

        GraphqlDevise::Types::QueryType.field(action, resolver: resolver)
      end

-      if (used_queries.blank? || additional_queries.present?) && GraphqlDevise::Types::QueryType.fields.blank?


I think the names are better if you are more explicit about what the variable holds

mcelicalderon

I don't think we should change file structure and not take advantage of autoload by using rails engine dir structure.

mcelicalderon · 2020-03-08T03:58:00Z

        resource.pluralize.underscore.tr('/', '_').to_sym,
        module: :devise,
-        skip:   [:sessions, :registrations, :passwords, :confirmations, :omniauth_callbacks, :unlocks, :invitations]
+        skip:   [DEVISE_OPERATIONS]


Constant already an array

mcelicalderon · 2020-03-08T03:59:53Z

+    end
+
+    def call
+      result = @default


No need for this assignment if line 21 has no conditional assignment

mcelicalderon · 2020-03-08T04:01:36Z

-
-      path         = opts.fetch(:at, '/graphql_auth')
-      mapping_name = resource.underscore.tr('/', '_').to_sym
+      validate_gql_devise_operations!(param_operations)


Still think you should call the new class method here directly instead of adding another method to rails router

mcelicalderon · 2020-03-08T04:07:55Z

      end
-      additional_queries.each do |action, resolver|
+
+      # if all_mutations.present? && GraphqlDevise::Schema.try(:mutation).nil?


I don't think try should replace the version check. I think all versions respond to the method actually. Plus the mutation method behaves differently on gql 1.10

mcelicalderon · 2020-03-14T18:40:27Z

    end

-    before { post_request }
+    before { post_request('/api/v1/guest/graphql_auth') }


Is this necessary?

mcelicalderon · 2020-03-14T18:41:32Z

    end

    it 'works without the confirmable module' do
-      expect { post_request }.to change(Guest, :count).from(0).to(1)


I don't think by is an improvement over the explicit start and end count you expect

mcelicalderon · 2020-03-14T18:42:06Z

+  describe '.call' do
+    subject { described_class.call(default: default, custom: custom, only: only, skipped: skipped) }
+
+    let(:op_class_1) { Class.new }


numbers in variables

mcelicalderon · 2020-03-14T18:42:38Z

+      context 'when there are custom operations' do
+        let(:custom) { { operation_1:   op_class_3, bad_operation: GraphQL::Schema::Resolver } }
+
+        it { is_expected.to eq(operation_1: op_class_3, operation_2: op_class_2) }


all numbers in variables, we should add the rule to rubocop

mcelicalderon · 2020-03-14T19:04:55Z

-
-      if (used_mutations.present? || additional_mutations.present?) &&
-         (Gem::Version.new(GraphQL::VERSION) <= Gem::Version.new('1.10.0') || GraphqlDevise::Schema.mutation.nil?)
-        GraphqlDevise::Schema.mutation(GraphqlDevise::Types::MutationType)


There was a reason for this not no be added if the mutation type has no mutations. You will get an error when making a request saying that the mutation type needs at least one field

Add operation sanitizer class Refactor sanitizer to have single responsability Add operations checker class Add mutations and queries preparer

Load default types, mutations and resolvers explicitely Use try instead of gem version to check for mutation type

mcelicalderon

New test case doesn't really test the schema having absolutely no mutations

mcelicalderon reviewed Feb 17, 2020

View reviewed changes

mcelicalderon reviewed Mar 8, 2020

View reviewed changes

00dav00 force-pushed the refactor_routes branch from 0dd8007 to f5d5882 Compare March 14, 2020 13:36

mcelicalderon reviewed Mar 14, 2020

View reviewed changes

David Revelo added 3 commits March 15, 2020 09:45

Split mapper functionally into classes

dcc5993

Add operation sanitizer class Refactor sanitizer to have single responsability Add operations checker class Add mutations and queries preparer

Refactor mapper

0e5dfdd

Load default types, mutations and resolvers explicitely Use try instead of gem version to check for mutation type

Move mutation assign

e24a5b8

00dav00 force-pushed the refactor_routes branch 3 times, most recently from 0d298f4 to 927858c Compare March 15, 2020 14:54

mcelicalderon approved these changes Mar 15, 2020

View reviewed changes

mcelicalderon requested changes Mar 15, 2020

View reviewed changes

00dav00 force-pushed the refactor_routes branch 3 times, most recently from e9f7f67 to 144d07b Compare March 21, 2020 13:10

Add types to the schema in the mapper

c08f7d2

00dav00 force-pushed the refactor_routes branch from 3dc245e to c08f7d2 Compare March 21, 2020 15:56

Merge branch 'master' into refactor_routes

b73d3ca

mcelicalderon force-pushed the refactor_routes branch from 1a5eeb6 to b73d3ca Compare March 21, 2020 17:12

mcelicalderon changed the title ~~Add operation sanitizer class~~ Refactor routes mount method Mar 21, 2020

mcelicalderon merged commit e466279 into master Mar 21, 2020

mcelicalderon deleted the refactor_routes branch March 21, 2020 17:54

Conversation

00dav00 commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcelicalderon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcelicalderon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

00dav00 commented Feb 8, 2020 •

edited

Loading