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

Hide signup path unless the instance allows new registration. #3055

Merged
merged 1 commit into from May 15, 2017

Conversation

Projects
None yet
3 participants
@treby
Contributor

treby commented May 14, 2017

When an instance does not allow register account ( open_registrations = false ),

Before
2017-05-14 23 31 10

After
2017-05-14 23 04 06

@ykzts

This comment has been minimized.

Show comment
Hide comment
@ykzts

ykzts May 14, 2017

Collaborator

What happens to languages ​​other than en and ja with this change?

Collaborator

ykzts commented May 14, 2017

What happens to languages ​​other than en and ja with this change?

@treby

This comment has been minimized.

Show comment
Hide comment
@treby

treby May 14, 2017

Contributor

@ykzts Thank you for your comment!

Actually, I've tried to add other language's keys by i18n-tasks add-missing, then there is so big diff (although not pr's related keys are added). So I wondered if is there any guidelines for new i18n key?

Of course, I'm happy to add other languages key by en translation, or just split existing landing_strip_html value by my hand (but since I'm not good at many languages, I may break existing translation...).

What can I do to improve this pr?

Contributor

treby commented May 14, 2017

@ykzts Thank you for your comment!

Actually, I've tried to add other language's keys by i18n-tasks add-missing, then there is so big diff (although not pr's related keys are added). So I wondered if is there any guidelines for new i18n key?

Of course, I'm happy to add other languages key by en translation, or just split existing landing_strip_html value by my hand (but since I'm not good at many languages, I may break existing translation...).

What can I do to improve this pr?

link_to_root_path: link_to(content_tag(:strong, site_hostname), root_path),
sign_up_path: new_user_registration_path)
link_to_root_path: link_to(content_tag(:strong, site_hostname), root_path))
- if open_registrations?

This comment has been minimized.

@ykzts

ykzts May 14, 2017

Collaborator

It would be better to use InstancePresenter like about/_links.html.haml.

@ykzts

ykzts May 14, 2017

Collaborator

It would be better to use InstancePresenter like about/_links.html.haml.

This comment has been minimized.

@treby

treby May 15, 2017

Contributor

hmm...
To make this pr, at first, I tried to use InstancePresenter to get the configuration (and DRY).
but IMHO, it's not a very good idea to provide the presenter instance only for _landing_strip.html.haml .

Now, _landing_strip.html.haml is called from

  • app/views/accounts/show.html.haml ( app/controllers/accounts_controller.rb )
  • app/views/stream_entries/show.html.haml (app/controllers/stream_entries_controller.rb )

There two controllers are completely difference (unlike about_controller), so I think it's hard for developers to know how controller's InstanceRepresenter instance variable is used.

Of course, I'll add both controllers by your request, but my opinion is above.

@treby

treby May 15, 2017

Contributor

hmm...
To make this pr, at first, I tried to use InstancePresenter to get the configuration (and DRY).
but IMHO, it's not a very good idea to provide the presenter instance only for _landing_strip.html.haml .

Now, _landing_strip.html.haml is called from

  • app/views/accounts/show.html.haml ( app/controllers/accounts_controller.rb )
  • app/views/stream_entries/show.html.haml (app/controllers/stream_entries_controller.rb )

There two controllers are completely difference (unlike about_controller), so I think it's hard for developers to know how controller's InstanceRepresenter instance variable is used.

Of course, I'll add both controllers by your request, but my opinion is above.

This comment has been minimized.

@treby

treby May 15, 2017

Contributor

This is an patch image for your request (that I'm thinking).

diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb
index 8eda9633..319955cf 100644
--- a/app/controllers/accounts_controller.rb
+++ b/app/controllers/accounts_controller.rb
@@ -6,6 +6,7 @@ class AccountsController < ApplicationController
   def show
     respond_to do |format|
       format.html do
+        @instance_presenter = InstancePresenter.new
         @statuses = @account.statuses.permitted_for(@account, current_account).order('id desc').paginate_by_max_id(20, params[:max_id], params[:since_id])
         @statuses = cache_collection(@statuses, Status)
       end
diff --git a/app/controllers/stream_entries_controller.rb b/app/controllers/stream_entries_controller.rb
index a9ee7350..64277566 100644
--- a/app/controllers/stream_entries_controller.rb
+++ b/app/controllers/stream_entries_controller.rb
@@ -13,6 +13,7 @@ class StreamEntriesController < ApplicationController
       format.html do
         return gone if @stream_entry.activity.nil?
 
+        @instance_presenter = InstancePresenter.new
         if @stream_entry.activity_type == 'Status'
           @ancestors   = @stream_entry.activity.reply? ? cache_collection(@stream_entry.activity.ancestors(current_account), Status) : []
           @descendants = cache_collection(@stream_entry.activity.descendants(current_account), Status)
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 8f1cd8fc..92ffac33 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -9,10 +9,6 @@ module ApplicationHelper
     !user_signed_in? && !single_user_mode?
   end
 
-  def open_registrations?
-    Setting.open_registrations
-  end
-
   def add_rtl_body_class(other_classes)
     other_classes = "#{other_classes} rtl" if [:ar, :fa, :he].include?(I18n.locale)
     other_classes
diff --git a/app/views/accounts/show.html.haml b/app/views/accounts/show.html.haml
index a1904910..e21827da 100644
--- a/app/views/accounts/show.html.haml
+++ b/app/views/accounts/show.html.haml
@@ -9,7 +9,7 @@
   = render 'og', account: @account, url: short_account_url(@account, only_path: false)
 
 - if show_landing_strip?
-  = render partial: 'shared/landing_strip', locals: { account: @account }
+  = render partial: 'shared/landing_strip', locals: { account: @account, instance_presenter: @instance_presenter }
 
 .h-feed
   %data.p-name{ value: "#{@account.username} on #{site_hostname}" }/
diff --git a/app/views/shared/_landing_strip.html.haml b/app/views/shared/_landing_strip.html.haml
index 3cc61a2c..77b2678a 100644
--- a/app/views/shared/_landing_strip.html.haml
+++ b/app/views/shared/_landing_strip.html.haml
@@ -2,5 +2,5 @@
   = t('landing_strip_html',
     name: content_tag(:span, display_name(account), class: :emojify),
     link_to_root_path: link_to(content_tag(:strong, site_hostname), root_path))
-  - if open_registrations?
+  - if instance_presenter&.open_registrations
     = t('landing_strip_signup_html', sign_up_path: new_user_registration_path)
diff --git a/app/views/stream_entries/show.html.haml b/app/views/stream_entries/show.html.haml
index d01e82af..8ba16b78 100644
--- a/app/views/stream_entries/show.html.haml
+++ b/app/views/stream_entries/show.html.haml
@@ -13,7 +13,7 @@
   %meta{ property: 'twitter:card', content: 'summary' }/
 
 - if show_landing_strip?
-  = render partial: 'shared/landing_strip', locals: { account: @stream_entry.account }
+  = render partial: 'shared/landing_strip', locals: { account: @stream_entry.account, instance_presenter: @instance_presenter }
 
 .activity-stream.activity-stream-headless.h-entry
   = render partial: "stream_entries/#{@type}", locals: { @type.to_sym => @stream_entry.activity, include_threads: true }
@treby

treby May 15, 2017

Contributor

This is an patch image for your request (that I'm thinking).

diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb
index 8eda9633..319955cf 100644
--- a/app/controllers/accounts_controller.rb
+++ b/app/controllers/accounts_controller.rb
@@ -6,6 +6,7 @@ class AccountsController < ApplicationController
   def show
     respond_to do |format|
       format.html do
+        @instance_presenter = InstancePresenter.new
         @statuses = @account.statuses.permitted_for(@account, current_account).order('id desc').paginate_by_max_id(20, params[:max_id], params[:since_id])
         @statuses = cache_collection(@statuses, Status)
       end
diff --git a/app/controllers/stream_entries_controller.rb b/app/controllers/stream_entries_controller.rb
index a9ee7350..64277566 100644
--- a/app/controllers/stream_entries_controller.rb
+++ b/app/controllers/stream_entries_controller.rb
@@ -13,6 +13,7 @@ class StreamEntriesController < ApplicationController
       format.html do
         return gone if @stream_entry.activity.nil?
 
+        @instance_presenter = InstancePresenter.new
         if @stream_entry.activity_type == 'Status'
           @ancestors   = @stream_entry.activity.reply? ? cache_collection(@stream_entry.activity.ancestors(current_account), Status) : []
           @descendants = cache_collection(@stream_entry.activity.descendants(current_account), Status)
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 8f1cd8fc..92ffac33 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -9,10 +9,6 @@ module ApplicationHelper
     !user_signed_in? && !single_user_mode?
   end
 
-  def open_registrations?
-    Setting.open_registrations
-  end
-
   def add_rtl_body_class(other_classes)
     other_classes = "#{other_classes} rtl" if [:ar, :fa, :he].include?(I18n.locale)
     other_classes
diff --git a/app/views/accounts/show.html.haml b/app/views/accounts/show.html.haml
index a1904910..e21827da 100644
--- a/app/views/accounts/show.html.haml
+++ b/app/views/accounts/show.html.haml
@@ -9,7 +9,7 @@
   = render 'og', account: @account, url: short_account_url(@account, only_path: false)
 
 - if show_landing_strip?
-  = render partial: 'shared/landing_strip', locals: { account: @account }
+  = render partial: 'shared/landing_strip', locals: { account: @account, instance_presenter: @instance_presenter }
 
 .h-feed
   %data.p-name{ value: "#{@account.username} on #{site_hostname}" }/
diff --git a/app/views/shared/_landing_strip.html.haml b/app/views/shared/_landing_strip.html.haml
index 3cc61a2c..77b2678a 100644
--- a/app/views/shared/_landing_strip.html.haml
+++ b/app/views/shared/_landing_strip.html.haml
@@ -2,5 +2,5 @@
   = t('landing_strip_html',
     name: content_tag(:span, display_name(account), class: :emojify),
     link_to_root_path: link_to(content_tag(:strong, site_hostname), root_path))
-  - if open_registrations?
+  - if instance_presenter&.open_registrations
     = t('landing_strip_signup_html', sign_up_path: new_user_registration_path)
diff --git a/app/views/stream_entries/show.html.haml b/app/views/stream_entries/show.html.haml
index d01e82af..8ba16b78 100644
--- a/app/views/stream_entries/show.html.haml
+++ b/app/views/stream_entries/show.html.haml
@@ -13,7 +13,7 @@
   %meta{ property: 'twitter:card', content: 'summary' }/
 
 - if show_landing_strip?
-  = render partial: 'shared/landing_strip', locals: { account: @stream_entry.account }
+  = render partial: 'shared/landing_strip', locals: { account: @stream_entry.account, instance_presenter: @instance_presenter }
 
 .activity-stream.activity-stream-headless.h-entry
   = render partial: "stream_entries/#{@type}", locals: { @type.to_sym => @stream_entry.activity, include_threads: true }

This comment has been minimized.

@ykzts

ykzts May 15, 2017

Collaborator

Well surely it is.

@ykzts

ykzts May 15, 2017

Collaborator

Well surely it is.

@ykzts

This comment has been minimized.

Show comment
Hide comment
@ykzts

ykzts May 14, 2017

Collaborator

I think that it is okay to temporarily make it into English.

Collaborator

ykzts commented May 14, 2017

I think that it is okay to temporarily make it into English.

@ykzts

This comment has been minimized.

Show comment
Hide comment
@ykzts

ykzts May 14, 2017

Collaborator

Of course you should divide within the range you can split.

Collaborator

ykzts commented May 14, 2017

Of course you should divide within the range you can split.

@treby

This comment has been minimized.

Show comment
Hide comment
@treby

treby May 15, 2017

Contributor

Okay I'll try it.

Contributor

treby commented May 15, 2017

Okay I'll try it.

@treby

This comment has been minimized.

Show comment
Hide comment
@treby

treby May 15, 2017

Contributor

@ykzts Added all language's translation and replied your review

Contributor

treby commented May 15, 2017

@ykzts Added all language's translation and replied your review

@ykzts

ykzts approved these changes May 15, 2017

@Gargron Gargron merged commit cb50ecd into tootsuite:master May 15, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@treby treby deleted the treby:split-signup-html branch May 15, 2017

Gargron added a commit that referenced this pull request May 22, 2017

Gargron added a commit that referenced this pull request May 22, 2017

Gargron added a commit that referenced this pull request May 23, 2017

Gargron added a commit that referenced this pull request May 23, 2017

gol-cha added a commit to gol-cha/mastodon that referenced this pull request May 29, 2017

gol-cha added a commit to gol-cha/mastodon that referenced this pull request May 29, 2017

orekyuu added a commit to orekyuu/mastodon that referenced this pull request May 31, 2017

orekyuu added a commit to orekyuu/mastodon that referenced this pull request May 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment