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

Updated layout of ichain sign in page #1110

Merged
merged 2 commits into from Aug 16, 2016

Conversation

jmks
Copy link
Contributor

@jmks jmks commented Jul 28, 2016

Fix for #1098

I basically merged the content of the devise ichain views with the current database_authenticatable sign in page.

I could sign in successfully using ichain test mode. When not in test mode, how can I test this locally? Do I just need to create an openSUSE account and try to sign in, or is there more setup? (I am not familiar with ichain at all)

@@ -9,3 +9,7 @@
- if devise_mapping.lockable? && resource_class.unlock_strategy_enabled?(:email) && controller_name != 'unlocks'
= link_to "Didn't receive unlock instructions?", new_unlock_path(resource_name)
%br

- if devise_mapping.ichain_registerable?
= link_to "Sign up", new_ichain_registration_path(resource_name)
Copy link
Contributor

@differentreality differentreality Aug 4, 2016

Choose a reason for hiding this comment

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

Sign up is not possible when ichain is enabled.

Copy link
Member

@ChrisBr ChrisBr Aug 8, 2016

Choose a reason for hiding this comment

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

Hm, new_ichain_registration_path(resource_name) should redirect you to the sign up page of the iChain provider, shouldn't it?

Copy link
Contributor

@differentreality differentreality Aug 11, 2016

Choose a reason for hiding this comment

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

Yes, true, that's what happens in production.
The correct path though is new_user_ichain_registration_path

@jmks Where did you find the path you are using new_ichain_registration_path ?

Copy link
Contributor Author

@jmks jmks Aug 11, 2016

Choose a reason for hiding this comment

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

@differentreality It's a helper in the devise_ichain_authenticatable gem that is used in their views. It seems to be used in one other place:

app/views/layouts/_navigation.html.haml
47:            %li{:class=> "#{active_nav_li(new_ichain_registration_path('user'))}"}
48:              = link_to(new_ichain_registration_path('user')) do

Should I update all usages of new_ichain_registration_path to new_user_ichain_registration_path?

Copy link
Contributor

@differentreality differentreality Aug 11, 2016

Choose a reason for hiding this comment

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

Oh, you are right... Well in that case no, you don't have to change it.

@differentreality
Copy link
Contributor

@differentreality differentreality commented Aug 4, 2016

Hey @jmks, nice job, thank you for the contribution!

I don't think you can test that locally (outside of test mode), @ChrisBr ?

@differentreality differentreality self-assigned this Aug 4, 2016
%h3.panel-title
Sign in
.panel-body
= semantic_form_for "", method: :post, enctype: 'application/x-www-form-urlencoded', url: @login_url do |f|
Copy link
Contributor

@differentreality differentreality Aug 11, 2016

Choose a reason for hiding this comment

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

For styling purposes, please use single quotes here.

%h3.panel-title
Sign in (TEST MODE)
.panel-body
= semantic_form_for "", method: :post, enctype: 'application/x-www-form-urlencoded', url: @login_url do |f|
Copy link
Contributor

@differentreality differentreality Aug 11, 2016

Choose a reason for hiding this comment

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

These quotes too?

@differentreality
Copy link
Contributor

@differentreality differentreality commented Aug 16, 2016

👍 thank you for the contribution @jmks!

@differentreality differentreality merged commit 10121ae into openSUSE:master Aug 16, 2016
3 checks passed
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.

None yet

3 participants