From 519119f657cf97ec187008a28dba00c1125a9292 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 11 Apr 2018 19:35:09 +0900 Subject: [PATCH 1/3] Paginate ancestor statuses in public page (#7102) This also limits the statuses returned by API, but pagination is not implemented in Web API yet. I still expect it brings user experience better than making a user wait to fetch all ancestor statuses and flooding the column with them. --- app/controllers/api/v1/statuses_controller.rb | 2 +- app/controllers/statuses_controller.rb | 7 +++- .../styles/mastodon/stream_entries.scss | 14 ++++++- .../concerns/status_threading_concern.rb | 24 ++++++++---- app/views/stream_entries/_status.html.haml | 4 ++ .../concerns/status_threading_concern_spec.rb | 38 +++++++++++++++---- .../stream_entries/show.html.haml_spec.rb | 2 +- 7 files changed, 70 insertions(+), 21 deletions(-) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 28c28592ade..e982413236c 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -17,7 +17,7 @@ class Api::V1::StatusesController < Api::BaseController end def context - ancestors_results = @status.in_reply_to_id.nil? ? [] : @status.ancestors(current_account) + ancestors_results = @status.in_reply_to_id.nil? ? [] : @status.ancestors(DEFAULT_STATUSES_LIMIT, current_account) descendants_results = @status.descendants(current_account) loaded_ancestors = cache_collection(ancestors_results, Status) loaded_descendants = cache_collection(descendants_results, Status) diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index 45226c8d2ae..41f098a4376 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -4,6 +4,8 @@ class StatusesController < ApplicationController include SignatureAuthentication include Authorization + ANCESTORS_LIMIT = 20 + layout 'public' before_action :set_account @@ -16,8 +18,9 @@ class StatusesController < ApplicationController def show respond_to do |format| format.html do - @ancestors = @status.reply? ? cache_collection(@status.ancestors(current_account), Status) : [] - @descendants = cache_collection(@status.descendants(current_account), Status) + @ancestors = @status.reply? ? cache_collection(@status.ancestors(ANCESTORS_LIMIT, current_account), Status) : [] + @next_ancestor = @ancestors.size < ANCESTORS_LIMIT ? nil : @ancestors.shift + @descendants = cache_collection(@status.descendants(current_account), Status) render 'stream_entries/show' end diff --git a/app/javascript/styles/mastodon/stream_entries.scss b/app/javascript/styles/mastodon/stream_entries.scss index 442b143a048..dfdc48d0610 100644 --- a/app/javascript/styles/mastodon/stream_entries.scss +++ b/app/javascript/styles/mastodon/stream_entries.scss @@ -6,7 +6,8 @@ background: $simple-background-color; .detailed-status.light, - .status.light { + .status.light, + .more.light { border-bottom: 1px solid $ui-secondary-color; animation: none; } @@ -290,6 +291,17 @@ text-decoration: underline; } } + + .more { + color: $classic-primary-color; + display: block; + padding: 14px; + text-align: center; + + &:not(:hover) { + text-decoration: none; + } + } } .embed { diff --git a/app/models/concerns/status_threading_concern.rb b/app/models/concerns/status_threading_concern.rb index b539ba10e2d..fffc095ee0f 100644 --- a/app/models/concerns/status_threading_concern.rb +++ b/app/models/concerns/status_threading_concern.rb @@ -3,8 +3,8 @@ module StatusThreadingConcern extend ActiveSupport::Concern - def ancestors(account = nil) - find_statuses_from_tree_path(ancestor_ids, account) + def ancestors(limit, account = nil) + find_statuses_from_tree_path(ancestor_ids(limit), account) end def descendants(account = nil) @@ -13,14 +13,21 @@ module StatusThreadingConcern private - def ancestor_ids - Rails.cache.fetch("ancestors:#{id}") do - ancestor_statuses.pluck(:id) + def ancestor_ids(limit) + key = "ancestors:#{id}" + ancestors = Rails.cache.fetch(key) + + if ancestors.nil? || ancestors[:limit] < limit + ids = ancestor_statuses(limit).pluck(:id).reverse! + Rails.cache.write key, limit: limit, ids: ids + ids + else + ancestors[:ids].last(limit) end end - def ancestor_statuses - Status.find_by_sql([<<-SQL.squish, id: in_reply_to_id]) + def ancestor_statuses(limit) + Status.find_by_sql([<<-SQL.squish, id: in_reply_to_id, limit: limit]) WITH RECURSIVE search_tree(id, in_reply_to_id, path) AS ( SELECT id, in_reply_to_id, ARRAY[id] @@ -34,7 +41,8 @@ module StatusThreadingConcern ) SELECT id FROM search_tree - ORDER BY path DESC + ORDER BY path + LIMIT :limit SQL end diff --git a/app/views/stream_entries/_status.html.haml b/app/views/stream_entries/_status.html.haml index e2e1fdd1216..2d0dafcb7fd 100644 --- a/app/views/stream_entries/_status.html.haml +++ b/app/views/stream_entries/_status.html.haml @@ -14,6 +14,10 @@ entry_classes = h_class + ' ' + mf_classes + ' ' + style_classes - if status.reply? && include_threads + - if @next_ancestor + .entry{ class: entry_classes } + = link_to short_account_status_url(@next_ancestor.account.username, @next_ancestor), class: 'more light' do + = t('statuses.show_more') = render partial: 'stream_entries/status', collection: @ancestors, as: :status, locals: { is_predecessor: true, direct_reply_id: status.in_reply_to_id } .entry{ class: entry_classes } diff --git a/spec/models/concerns/status_threading_concern_spec.rb b/spec/models/concerns/status_threading_concern_spec.rb index 62f5f6e317e..b8ebdd58cda 100644 --- a/spec/models/concerns/status_threading_concern_spec.rb +++ b/spec/models/concerns/status_threading_concern_spec.rb @@ -14,34 +14,34 @@ describe StatusThreadingConcern do let!(:viewer) { Fabricate(:account, username: 'viewer') } it 'returns conversation history' do - expect(reply3.ancestors).to include(status, reply1, reply2) + expect(reply3.ancestors(4)).to include(status, reply1, reply2) end it 'does not return conversation history user is not allowed to see' do reply1.update(visibility: :private) status.update(visibility: :direct) - expect(reply3.ancestors(viewer)).to_not include(reply1, status) + expect(reply3.ancestors(4, viewer)).to_not include(reply1, status) end it 'does not return conversation history from blocked users' do viewer.block!(jeff) - expect(reply3.ancestors(viewer)).to_not include(reply1) + expect(reply3.ancestors(4, viewer)).to_not include(reply1) end it 'does not return conversation history from muted users' do viewer.mute!(jeff) - expect(reply3.ancestors(viewer)).to_not include(reply1) + expect(reply3.ancestors(4, viewer)).to_not include(reply1) end it 'does not return conversation history from silenced and not followed users' do jeff.update(silenced: true) - expect(reply3.ancestors(viewer)).to_not include(reply1) + expect(reply3.ancestors(4, viewer)).to_not include(reply1) end it 'does not return conversation history from blocked domains' do viewer.block_domain!('example.com') - expect(reply3.ancestors(viewer)).to_not include(reply2) + expect(reply3.ancestors(4, viewer)).to_not include(reply2) end it 'ignores deleted records' do @@ -49,10 +49,32 @@ describe StatusThreadingConcern do second_status = Fabricate(:status, thread: first_status, account: alice) # Create cache and delete cached record - second_status.ancestors + second_status.ancestors(4) first_status.destroy - expect(second_status.ancestors).to eq([]) + expect(second_status.ancestors(4)).to eq([]) + end + + it 'can return more records than previously requested' do + first_status = Fabricate(:status, account: bob) + second_status = Fabricate(:status, thread: first_status, account: alice) + third_status = Fabricate(:status, thread: second_status, account: alice) + + # Create cache + second_status.ancestors(1) + + expect(third_status.ancestors(2)).to eq([first_status, second_status]) + end + + it 'can return fewer records than previously requested' do + first_status = Fabricate(:status, account: bob) + second_status = Fabricate(:status, thread: first_status, account: alice) + third_status = Fabricate(:status, thread: second_status, account: alice) + + # Create cache + second_status.ancestors(2) + + expect(third_status.ancestors(1)).to eq([second_status]) end end diff --git a/spec/views/stream_entries/show.html.haml_spec.rb b/spec/views/stream_entries/show.html.haml_spec.rb index 59ea409907d..6074bbc2efa 100644 --- a/spec/views/stream_entries/show.html.haml_spec.rb +++ b/spec/views/stream_entries/show.html.haml_spec.rb @@ -48,7 +48,7 @@ describe 'stream_entries/show.html.haml', without_verify_partial_doubles: true d assign(:stream_entry, reply.stream_entry) assign(:account, alice) assign(:type, reply.stream_entry.activity_type.downcase) - assign(:ancestors, reply.stream_entry.activity.ancestors(bob) ) + assign(:ancestors, reply.stream_entry.activity.ancestors(1, bob) ) assign(:descendants, reply.stream_entry.activity.descendants(bob)) render From 12f5f13fab1023c3cb2a50c7bcb11f281c7984fb Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 11 Apr 2018 20:42:50 +0200 Subject: [PATCH 2/3] Place privacy dropdown menu top if it is closer to the bottom of the viewport (#7106) --- .../compose/components/privacy_dropdown.js | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/app/javascript/mastodon/features/compose/components/privacy_dropdown.js b/app/javascript/mastodon/features/compose/components/privacy_dropdown.js index e5de13178d6..6b22ba84a53 100644 --- a/app/javascript/mastodon/features/compose/components/privacy_dropdown.js +++ b/app/javascript/mastodon/features/compose/components/privacy_dropdown.js @@ -32,6 +32,10 @@ class PrivacyDropdownMenu extends React.PureComponent { onChange: PropTypes.func.isRequired, }; + state = { + mounted: false, + }; + handleDocumentClick = e => { if (this.node && !this.node.contains(e.target)) { this.props.onClose(); @@ -54,6 +58,7 @@ class PrivacyDropdownMenu extends React.PureComponent { componentDidMount () { document.addEventListener('click', this.handleDocumentClick, false); document.addEventListener('touchend', this.handleDocumentClick, listenerOptions); + this.setState({ mounted: true }); } componentWillUnmount () { @@ -66,12 +71,16 @@ class PrivacyDropdownMenu extends React.PureComponent { } render () { + const { mounted } = this.state; const { style, items, value } = this.props; return ( {({ opacity, scaleX, scaleY }) => ( -
+ // It should not be transformed when mounting because the resulting + // size will be used to determine the coordinate of the menu by + // react-overlays +
{items.map(item => (
@@ -107,9 +116,10 @@ export default class PrivacyDropdown extends React.PureComponent { state = { open: false, + placement: null, }; - handleToggle = () => { + handleToggle = ({ target }) => { if (this.props.isUserTouching()) { if (this.state.open) { this.props.onModalClose(); @@ -120,6 +130,8 @@ export default class PrivacyDropdown extends React.PureComponent { }); } } else { + const { top } = target.getBoundingClientRect(); + this.setState({ placement: top * 2 < innerHeight ? 'bottom' : 'top' }); this.setState({ open: !this.state.open }); } } @@ -136,7 +148,7 @@ export default class PrivacyDropdown extends React.PureComponent { handleKeyDown = e => { switch(e.key) { case 'Enter': - this.handleToggle(); + this.handleToggle(e); break; case 'Escape': this.handleClose(); @@ -165,7 +177,7 @@ export default class PrivacyDropdown extends React.PureComponent { render () { const { value, intl } = this.props; - const { open } = this.state; + const { open, placement } = this.state; const valueOption = this.options.find(item => item.value === value); @@ -185,7 +197,7 @@ export default class PrivacyDropdown extends React.PureComponent { />
- + Date: Wed, 11 Apr 2018 21:40:38 +0200 Subject: [PATCH 3/3] update gem, test pam authentication (#7028) * update gem, test pam authentication * add description for test parameters * fix inclusion of optional group --- .env.test | 4 ++ .travis.yml | 3 +- Gemfile | 2 +- Gemfile.lock | 8 +-- config/environments/test.rb | 11 ++++ .../auth/sessions_controller_spec.rb | 51 +++++++++++++++++++ 6 files changed, 73 insertions(+), 6 deletions(-) diff --git a/.env.test b/.env.test index b57f52e309b..7da76f8ef88 100644 --- a/.env.test +++ b/.env.test @@ -1,3 +1,7 @@ # Federation LOCAL_DOMAIN=cb6e6126.ngrok.io LOCAL_HTTPS=true +# test pam authentication +PAM_ENABLED=true +PAM_DEFAULT_SERVICE=pam_test +PAM_CONTROLLED_SERVICE=pam_test_controlled diff --git a/.travis.yml b/.travis.yml index 989237a1923..2addd9ba2fe 100644 --- a/.travis.yml +++ b/.travis.yml @@ -23,6 +23,7 @@ env: - RAILS_ENV=test - NOKOGIRI_USE_SYSTEM_LIBRARIES=true - PARALLEL_TEST_PROCESSORS=2 + - ALLOW_NOPAM=true addons: postgresql: 9.4 @@ -47,7 +48,7 @@ services: install: - nvm install - - bundle install --path=vendor/bundle --without development production --retry=3 --jobs=16 + - bundle install --path=vendor/bundle --with pam_authentication --without development production --retry=3 --jobs=16 - yarn install before_script: diff --git a/Gemfile b/Gemfile index 4a5a166bdb2..7f9591d8d4d 100644 --- a/Gemfile +++ b/Gemfile @@ -33,7 +33,7 @@ gem 'devise', '~> 4.4' gem 'devise-two-factor', '~> 3.0' group :pam_authentication, optional: true do - gem 'devise_pam_authenticatable2', '~> 9.0' + gem 'devise_pam_authenticatable2', '~> 9.1' end gem 'net-ldap', '~> 0.10' diff --git a/Gemfile.lock b/Gemfile.lock index 0f5a1fb6ad7..5322b87461c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -146,9 +146,9 @@ GEM devise (~> 4.0) railties (< 5.2) rotp (~> 2.0) - devise_pam_authenticatable2 (9.0.0) + devise_pam_authenticatable2 (9.1.0) devise (>= 4.0.0) - rpam2 (~> 3.0) + rpam2 (~> 4.0) diff-lcs (1.3) docile (1.1.5) domain_name (0.5.20170404) @@ -464,7 +464,7 @@ GEM actionpack (>= 4.2.0, < 5.3) railties (>= 4.2.0, < 5.3) rotp (2.1.2) - rpam2 (3.1.0) + rpam2 (4.0.2) rqrcode (0.10.1) chunky_png (~> 1.0) rspec-core (3.7.0) @@ -639,7 +639,7 @@ DEPENDENCIES climate_control (~> 0.2) devise (~> 4.4) devise-two-factor (~> 3.0) - devise_pam_authenticatable2 (~> 9.0) + devise_pam_authenticatable2 (~> 9.1) doorkeeper (~> 4.2) dotenv-rails (~> 2.2) fabrication (~> 2.18) diff --git a/config/environments/test.rb b/config/environments/test.rb index 7d77a170e56..122634d5b8b 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -59,3 +59,14 @@ Rails.application.configure do end Paperclip::Attachment.default_options[:path] = "#{Rails.root}/spec/test_files/:class/:id_partition/:style.:extension" + +# set fake_data for pam, don't do real calls, just use fake data +if ENV['PAM_ENABLED'] == 'true' + Rpam2.fake_data = + { + usernames: Set['pam_user1', 'pam_user2'], + servicenames: Set['pam_test', 'pam_test_controlled'], + password: '123456', + env: { email: 'pam@example.com' } + } +end diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 88f0a4734d1..d5fed17d63d 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -48,6 +48,57 @@ RSpec.describe Auth::SessionsController, type: :controller do request.env['devise.mapping'] = Devise.mappings[:user] end + context 'using PAM authentication' do + context 'using a valid password' do + before do + post :create, params: { user: { email: "pam_user1", password: '123456' } } + end + + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end + + it 'logs the user in' do + expect(controller.current_user).to be_instance_of(User) + end + end + + context 'using an invalid password' do + before do + post :create, params: { user: { email: "pam_user1", password: 'WRONGPW' } } + end + + it 'shows a login error' do + expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') + end + + it "doesn't log the user in" do + expect(controller.current_user).to be_nil + end + end + + context 'using a valid email and existing user' do + let(:user) do + account = Fabricate.build(:account, username: 'pam_user1') + account.save!(validate: false) + user = Fabricate(:user, email: 'pam@example.com', password: nil, account: account) + user + end + + before do + post :create, params: { user: { email: user.email, password: '123456' } } + end + + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end + + it 'logs the user in' do + expect(controller.current_user).to eq user + end + end + end + context 'using password authentication' do let(:user) { Fabricate(:user, email: 'foo@bar.com', password: 'abcdefgh') }