Store routing context when mounting engine, Fix i18n path #14

grekko · 2018-03-03T08:26:49Z

Disclaimer: This is just a WIP proposal. I am not really sure where/how its best to store the routing configuration for later re-use. Also this needs a test :) Happy to contribute the test.

I noticed that Passwordless allows users to change the root path of the engine. E.g. from the README:

passwordless_for :users, at: '/', as: :auth

If the root path is changed though, the Mailer.magic_link fails to generate the proper token_sign_in_url.

The same happens e.g. in the passwordless/sessions/new.html.erb-view.

houndci-bot · 2018-03-03T08:26:57Z

app/mailers/passwordless/mailer.rb

-        @session.authenticatable_type.underscore.pluralize
-      @magic_link =
-        send(authenticatable_resource_name).token_sign_in_url(session.token)
+      @magic_link = send(Passwordless.router_context).token_sign_in_url(session.token)


Line is too long. [86/80]

houndci-bot · 2018-03-03T08:26:57Z

app/mailers/passwordless/mailer.rb

-        @session.authenticatable_type.underscore.pluralize
-      @magic_link =
-        send(authenticatable_resource_name).token_sign_in_url(session.token)
+      @magic_link = send(Passwordless.router_context).token_sign_in_url(session.token)


Line is too long. [86/80]

houndci-bot · 2018-03-03T08:33:19Z

app/mailers/passwordless/mailer.rb

-      @magic_link =
-        send(authenticatable_resource_name).token_sign_in_url(session.token)
+      @magic_link = send(Passwordless.router_context)
+                      .token_sign_in_url(session.token)


Align .token_sign_in_url with send(Passwordless.router_context) on line 13.

mikker · 2018-03-04T16:54:36Z

I actually have a friend stumble into this the other day and didn't have the time to investigate. Perfect! Thanks. I think I'd prefer to have the variable called mounted_at instead of routing_context. Or maybe have a Passwordless.context object with a nested .mounted_at. This gives us one place to save stuff like this if more shows up.

This is a very nice way to solve this problem 👍 thanks! A test case would be very welcome here as well.

houndci-bot · 2018-03-07T21:26:22Z

test/integration/navigation_test.rb

+      },
+      headers: { 'HTTP_USER_AGENT' => 'Mosaic v.1' }
+    assert_equal 200, status
+    assert response.body.include?('If we found you in the system, we have sent you an email.')


Line is too long. [94/80]

houndci-bot · 2018-03-07T21:26:22Z

test/integration/navigation_test.rb

+    assert_equal 'Not worthy!', flash['error']
+    follow_redirect!
+    assert_equal 200, status
+    assert_equal "/", path


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2018-03-07T21:26:22Z

test/integration/navigation_test.rb

-  # end
+  fixtures :users
+
+  test "the truth" do


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2018-03-07T21:26:22Z

test/integration/navigation_test.rb

+      },
+      headers: { 'HTTP_USER_AGENT' => 'Mosaic v.1' }
+    assert_equal 200, status
+    assert response.body.include?('If we found you in the system, we have sent you an email.')


Line is too long. [94/80]

houndci-bot · 2018-03-07T21:26:23Z

test/integration/navigation_test.rb

+    assert_equal 'Not worthy!', flash['error']
+    follow_redirect!
+    assert_equal 200, status
+    assert_equal "/", path


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2018-03-07T21:26:23Z

test/integration/navigation_test.rb

-  # end
+  fixtures :users
+
+  test "the truth" do


Prefer single-quoted strings when you don't need string interpolation or special symbols.

grekko · 2018-03-07T21:42:31Z

config/locales/en.yml

@@ -4,7 +4,7 @@ en:
    sessions:
      create:
        session_expired: 'Your session has expired, please sign in again.'
-        email_sent_if_record_found: "If we found you in the system, we've sent you an email."
+        email_sent_if_record_found: "If we found you in the system, we have sent you an email."


I changed wording since rails actionview escapes the rendered content of we've into some html entities which were hard to capture in the integration test.

Another solution would be to stub the I18n.translate-call or change the backend/translations just for the test.

Open for discuission here.

Wondered why you'd changed this. Reason sounds like a real hairball. Fine with this or third option below.

mikker · 2018-03-07T21:52:06Z

test/integration/navigation_test.rb

+      headers: { 'HTTP_USER_AGENT' => 'Mosaic v.1' }
+    assert_equal 200, status
+    assert response.body.include?(
+      'If we found you in the system, we have sent you an email.'


response.body.include?('If we found you in the system') could be another solution to the ' business.

Yea.. I tried swapping the i18n backend/translations or stubbing the calls to it but everything turned out to be extremely cumbersome for this single case.

So eventually I like to go w/ your proposed way of checking for the string partial.

mikker · 2018-03-07T21:53:58Z

test/integration/navigation_test.rb

+
+    assert_equal 200, status
+    assert_equal '/secret', path
+    assert_equal 'shhhh! secrets!', response.body


Glorious! 🎉

mikker · 2018-03-07T21:54:24Z

config/locales/en.yml

@@ -4,7 +4,7 @@ en:
    sessions:
      create:
        session_expired: 'Your session has expired, please sign in again.'
-        email_sent_if_record_found: "If we found you in the system, we've sent you an email."
+        email_sent_if_record_found: "If we found you in the system, we have sent you an email."


Wondered why you'd changed this. Reason sounds like a real hairball. Fine with this or third option below.

mikker · 2018-03-07T21:55:33Z

app/views/passwordless/sessions/new.html.erb

@@ -1,4 +1,4 @@
-<%= form_for @session, url: send(authenticatable_resource).sign_in_path do |f| %>
+<%= form_for @session, url: send(Passwordless.mounted_as).sign_in_path do |f| %>


Does this mean, we can remove that helper method?

Good catch. Removed it!

@mikker

Thanks @mikker

mikker · 2018-03-08T10:53:01Z

This is good 👍
Can I make you consolidate all the commits into one?

grekko · 2018-03-08T10:55:53Z

Can I make you consolidate all the commits into one?

Sure. But dont you just squash the commits when merging?

mikker · 2018-03-08T12:41:50Z

💙💚💛💜❤️

grekko · 2018-03-08T14:51:21Z

Glad to help 🤗

Store routing context when mounting for re-use in mailer/templates

2dbb14d

houndci-bot reviewed Mar 3, 2018

View reviewed changes

grekko mentioned this pull request Mar 3, 2018

Integration with Devise #9

Closed

Shorten line 🐺

0cd1f58

houndci-bot reviewed Mar 3, 2018

View reviewed changes

:) ok

f62c66e

houndci-bot reviewed Mar 7, 2018

View reviewed changes

Gregory Igelmund added 4 commits March 7, 2018 22:26

Adds basic integration test and fix i18n path

c57ea91

Fix lint issues

5a8eebb

Rename Passwordless.router_context to Passwordless.mounted_as

9a90c5b

Fix lint issue (line too long)

fbc7e17

grekko changed the title ~~Store routing context when mounting for re-use in mailer/templates~~ Store routing context when mounting engine, Fix i18n path Mar 7, 2018

grekko mentioned this pull request Mar 7, 2018

Fix i18n path for email_sent_if_record_found #15

Closed

Keep routes the way they were

6e5e00d

grekko commented Mar 7, 2018

View reviewed changes

mikker reviewed Mar 7, 2018

View reviewed changes

Gregory Igelmund added 2 commits March 8, 2018 10:44

Revert locale change and adjust integration test expectation

6283b61

Removes obsolete helper method

7392e9e

Thanks @mikker

mikker merged commit 3e17a8b into mikker:master Mar 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store routing context when mounting engine, Fix i18n path #14

Store routing context when mounting engine, Fix i18n path #14

grekko commented Mar 3, 2018 •

edited

Loading

houndci-bot Mar 3, 2018

houndci-bot Mar 3, 2018

houndci-bot Mar 3, 2018

mikker commented Mar 4, 2018 •

edited

Loading

houndci-bot Mar 7, 2018

houndci-bot Mar 7, 2018

houndci-bot Mar 7, 2018

houndci-bot Mar 7, 2018

houndci-bot Mar 7, 2018

houndci-bot Mar 7, 2018

grekko Mar 7, 2018

mikker Mar 7, 2018

mikker Mar 7, 2018

grekko Mar 8, 2018

mikker Mar 7, 2018

mikker Mar 7, 2018

mikker Mar 7, 2018

grekko Mar 8, 2018

mikker commented Mar 8, 2018

grekko commented Mar 8, 2018

mikker commented Mar 8, 2018

grekko commented Mar 8, 2018

		@@ -1,4 +1,4 @@
		<%= form_for @session, url: send(authenticatable_resource).sign_in_path do \|f\| %>
		<%= form_for @session, url: send(Passwordless.mounted_as).sign_in_path do \|f\| %>

Store routing context when mounting engine, Fix i18n path #14

Store routing context when mounting engine, Fix i18n path #14

Conversation

grekko commented Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikker commented Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikker commented Mar 8, 2018

grekko commented Mar 8, 2018

mikker commented Mar 8, 2018

grekko commented Mar 8, 2018

grekko commented Mar 3, 2018 •

edited

Loading

mikker commented Mar 4, 2018 •

edited

Loading