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]routes for rails api mode #69

Merged
merged 1 commit into from
Nov 6, 2021

Conversation

ihatov08
Copy link
Contributor

@ihatov08 ihatov08 commented Jul 25, 2017

This error occurred when using Rails API mode.

Started POST "/letter_opener/clear" for 172.18.0.1 at 2017-07-25 03:07:35 +0000
ActionController::RoutingError (No route matches [POST] "/letter_opener/clear"):

Plead add this routes.

  post 'clear'                 => 'letters#clear'
  post ':id'                   => 'letters#destroy'

related. #42

@fgrehm
Copy link
Owner

fgrehm commented Feb 18, 2020

Hey @ihatov08! Sorry for the long delay here.

I might have a need to use this gem on a side project with Rails API too and I'll give it a shot. I personally think we can just ignore the RESTfulness here. I'm fine with replacing the delete :clear with the post :clear and have an explicit post ':id/delete' just so that we use the same endpoint regardless of Rails API mode or not instead of duplicating the routes.

What do you and @pseudomuto think?

@ihatov08
Copy link
Contributor Author

@fgrehm
Thank you for your reply!
I think so too.
It is preferable that there is no route to the same endpoint.

LetterOpenerWeb::Engine.routes.draw do
  delete 'clear'                 => 'letters#clear',    :as => :clear_letters
  delete ':id'                   => 'letters#destroy',  :as => :delete_letter
  get    '/'                     => 'letters#index',    :as => :letters
  get    ':id(/:style)'          => 'letters#show',     :as => :letter
  get    ':id/attachments/:file' => 'letters#attachment'
  post   'clear'                 => 'letters#clear' # same endpoint line:2
  post   ':id'                   => 'letters#destroy' # same endpoint line:3
end

It would be nice if the following code works regardless of Rails mode

LetterOpenerWeb::Engine.routes.draw do
  post   'clear'                 => 'letters#clear'
  post   ':id'                   => 'letters#destroy'
  get    '/'                     => 'letters#index',    :as => :letters
  get    ':id(/:style)'          => 'letters#show',     :as => :letter
  get    ':id/attachments/:file' => 'letters#attachment'
end

@tgaff
Copy link

tgaff commented Feb 19, 2020

Here's my slightly hacky solution: 793b4e8

@fgrehm fgrehm mentioned this pull request Oct 20, 2021
16 tasks
@fgrehm
Copy link
Owner

fgrehm commented Oct 20, 2021

Hey @ihatov08 @tgaff, just a heads up that this is something that will come along with the upcoming 2.0 I got in the works, see the PR linked above for more.

If you have the time, it'd be great if one of you could give that branch a try, tks in advance and sorry for the long delay here!

@fgrehm
Copy link
Owner

fgrehm commented Oct 28, 2021

Just got a pre-release ready to go I'll just ping some folks for testing before a final release and this PR should auto close after code is on master.

Thanks for your contribution!

@ihatov08
Copy link
Contributor Author

@fgrehm

I tried next branch rails api mode.
But error occurred.

Action-Controller-Exception-caught (1)

https://github.com/ihatov08/letter_opener_web_rails_api

@ihatov08
Copy link
Contributor Author

@fgrehm

If we want to use letter_opener_web in rails api mode without modifying letter_opener_web, you need the following modifications.

  • fix undefined method flash=

ihatov08/letter_opener_web_rails_api@bbfb9a5

@fgrehm
Copy link
Owner

fgrehm commented Oct 30, 2021

Hey @ihatov08 , tks for the feedback. I'll take a look at that sample app and see if we can simplify this before final release and / or document how to integrate with rails-api apps

@fgrehm
Copy link
Owner

fgrehm commented Nov 1, 2021

Hey @ihatov08, I was not able to figure out why you had issue with flash msgs but the CSRF error should be fixed in 2.0.0.pre.beta with 3b4dc30

Please give that a shot when you get a chance and report back in #113 if things look good to you

@fgrehm fgrehm merged commit 5aba9a4 into fgrehm:master Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants