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

fix: Multi-word custom endpoint not respecting route format #400

Merged

Conversation

sebasjimenez10
Copy link
Contributor

This PR fixes an issue I experienced when working with multi-word custom endpoints. If you have a multi-word custom endpoint and you also have the self.route_format = :dasherized_route config in your base class, upon calling that custom_endpoint JsonApiClient doesn't respect the configuration selected and will use the underscored version of the endpoint name.

For instance:

Given,

class Pet < TestResource
  self.site = "http://example.com/"
  self.route_format = :dasherized_route

  custom_endpoint :related_pets, on: :member, request_method: :get
  custom_endpoint :vip_pets, on: :collection, request_method: :get
end

When calling Pet.vip_pets or pet = Pet.new(...); pet.related_pets the resulting endpoint being called will be either:
http://example.com/pets/vip_pets or http://example.com/pets/:id/related_pets

instead of http://example.com/pets/vip-pets or http://example.com/pets/:id/related-pets

Same issue applies for camelized_routes.

This PR fixes this by checking the route_format configuration attribute and making sure it is converted to the appropriate endpoint name, using ActiveSupport helper methods.

The methods defined in the instance of the class will still be underscored, so that we can still do Pet.vip_vets.

Happy to address any concerns or comments, thanks!

@sebasjimenez10 sebasjimenez10 changed the title [FIX] Multi-word custom endpoint not respecting route format FIX: Multi-word custom endpoint not respecting route format May 13, 2022
@sebasjimenez10
Copy link
Contributor Author

Hey @gaorlov 👋 giving this one a bump for you to take a look 🙏

@sebasjimenez10 sebasjimenez10 changed the title FIX: Multi-word custom endpoint not respecting route format [Fix] Multi-word custom endpoint not respecting route format Aug 11, 2022
@esbanarango
Copy link

What's the current blocker for merging this one? 🙏

@gaorlov
Copy link
Collaborator

gaorlov commented Aug 12, 2022

Hello! Thanks for taking the time to contribute! I'll take a closer look tomorrow. I'm the meantime, can you please add an entry to the changelog?

Thanks!

Greg

@sebasjimenez10
Copy link
Contributor Author

@gaorlov updated the changelog! 🙏

@sebasjimenez10
Copy link
Contributor Author

@gaorlov small bump! 🙂

@gaorlov
Copy link
Collaborator

gaorlov commented Aug 19, 2022

@sebasjimenez10 sorry about the delay. For reasons i don't yet understand the automated testing has stopped working. I'm working with the owners of the JsonAPiClient org to resolve this issue and will let you know when we can start merging again.

I appreciate your patience!

Greg

@sebasjimenez10
Copy link
Contributor Author

@gaorlov thank you so much for looking into it. No worries on the wait, I'm glad to contribute 🙏🏻

@sebasjimenez10
Copy link
Contributor Author

Hey @gaorlov, just dropping by and giving this one a bump! 🙏

@sebasjimenez10
Copy link
Contributor Author

Hey @gaorlov, hope you're doing good! Just wanted to bump this one!

@esbanarango
Copy link

🙏

@@ -348,6 +350,17 @@ def _build_connection(rebuild = false)
yield(conn) if block_given?
end
end

def _name_for_route_format(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be a method on name? like name.to_route(route_format)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaorlov name here is a Symbol, that would require a big refactor in other places as well and I believe that to be out of scope for this PR. But happy to discuss further to continue contributing 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you do like

module Routable
  def to_route(format: :dasherized_route)
    case format
    when :dasherized_route
      to_s.dasherize
    when :camelized_route
      to_s.camelize(:lower)
    else
      self
    end
  end
end
 
class Symbol
  include Routable
end

and then you can have name.to_route(route_format) and not have to do anything else to the code

Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

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

Looks great! thanks for contributing!
Please merge in main to get tests running in the PR
Thanks!

@sebasjimenez10 sebasjimenez10 force-pushed the sj/fix-multi-word-custom-endpoint branch from f0171fb to 8e07765 Compare June 6, 2023 03:56
@sebasjimenez10 sebasjimenez10 force-pushed the sj/fix-multi-word-custom-endpoint branch from 8e07765 to 9cf3d0f Compare June 6, 2023 04:23
@sebasjimenez10
Copy link
Contributor Author

@gaorlov done! Just rebased on top of master (main). Tests are looking good. Please let me know if there is anything else I need to do!

@sebasjimenez10 sebasjimenez10 changed the title [Fix] Multi-word custom endpoint not respecting route format fix: Multi-word custom endpoint not respecting route format Jun 6, 2023
@sebasjimenez10
Copy link
Contributor Author

@gaorlov small bump!

@sebasjimenez10
Copy link
Contributor Author

@gaorlov tiny bump on this one! 🙏

@gaorlov gaorlov merged commit 5d46961 into JsonApiClient:master May 2, 2024
16 checks passed
@gaorlov
Copy link
Collaborator

gaorlov commented May 2, 2024

thank you for you contributions and patience!
1.23.0 is now live with your changes

@sebasjimenez10 sebasjimenez10 deleted the sj/fix-multi-word-custom-endpoint branch May 6, 2024 16:52
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.

3 participants