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

View contributions publicly when peer-to-peer is on #810

Merged
merged 5 commits into from
Jan 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions app/controllers/contributions_controller.rb
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
# frozen_string_literal: true

class ContributionsController < ApplicationController
before_action :authenticate_user!, except: %i[combined_form respond thank_you]
before_action :authenticate_user!, unless: :peer_to_peer_mode?
before_action :set_contribution, only: %i[respond triage]

layout 'without_navbar', only: [:thank_you]

def index
@filter_types = FilterTypeBlueprint.render([ContributionType, Category, ServiceArea, UrgencyLevel, ContactMethod])
filter = BrowseFilter.new(filter_params, self)
@contributions = ContributionBlueprint.render(filter.contributions, **filter.options)
filter = BrowseFilter.new(filter_params)
@contributions = ContributionBlueprint.render(filter.contributions, contribution_blueprint_options)
respond_to do |format|
format.html
format.json { render inline: @contributions }
Expand Down Expand Up @@ -58,6 +58,16 @@ def triage_update

private

def peer_to_peer_mode?
@system_setting.peer_to_peer?
end

def contribution_blueprint_options
options = { respond_path: ->(id) { respond_contribution_path(id)} }
options[:view_path] = ->(id) { contribution_path(id) } if SystemSetting.current_settings&.peer_to_peer?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 62 above references @system_setting. Can we do that here as well?

options
end

def filter_params
return Hash.new unless allowed_params && allowed_params.to_h.any?
allowed_params.to_h.filter { |key, _v| BrowseFilter::ALLOWED_PARAMS.keys.include? key}.tap do |hash|
Expand Down
13 changes: 2 additions & 11 deletions app/models/browse_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,10 @@ class BrowseFilter
end.freeze
ALLOWED_MODEL_NAMES = ['Ask', 'Offer'].freeze

attr_reader :parameters, :context
attr_reader :parameters

def initialize(parameters, context = nil)
def initialize(parameters)
@parameters = parameters
@context = context
end

def contributions
Expand All @@ -38,14 +37,6 @@ def contributions
end
end

def options
return {} unless context

options = { respond_path: ->(id) { context.respond_contribution_path(id)} }
options[:view_path] = ->(id) { context.contribution_path(id) } if SystemSetting.current_settings&.peer_to_peer?
options
end

private

def filter(model)
Expand Down
77 changes: 59 additions & 18 deletions spec/requests/contributions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,68 @@
location_attributes: { zip: '12e45' },
}}

before { sign_in create(:user) }
let(:user) { create(:user) }

describe 'GET /index' do
it 'renders a successful response' do
create(:listing)
get contributions_url
expect(response).to be_successful
describe 'access' do
describe 'when logged in' do
before do
sign_in user
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, i don't love repeating buckets of examples across multiple contexts.

There are of course circumstances when that's appropriate, eg for cross-cutting concerns. But in many cases, it can quickly become difficult to know which examples really care about being tested in all contexts and which just happened to be lumped in out of convenience. This then can hinder future refactoring.

In this case, iirc the key thing to test between the three contexts is whether /contributions is accessible. I'd probably make that clear in a separate describe 'access' or similar block where that's the only thing being asserted. The examples below deal with details of the page which i don't think need to be re-run across contexts that are about authorization.

All that said, this is just my gut feeling -- i'm certainly fine with leaving it as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the tests as you've suggested


it 'renders the index of contributions' do
create(:listing)
get contributions_url
expect(response).to be_successful
end

it 'is shows a contribution response page' do
contribution = create(:listing)
get contribution_url(contribution)
expect(response).to be_successful
end
end

describe 'when logged out but p2p is enabled' do
before do
allow_any_instance_of(SystemSetting).to receive(:peer_to_peer?).and_return(true)
end

it 'renders the index of contributions' do
create(:listing)
get contributions_url
expect(response).to be_successful
end

# # This test is commented out because it probably should redirect to
# # a page that's different than would dispatchers etc. see
#
# it 'is shows a contribution response page' do
# contribution = create(:listing)
# get contribution_url(contribution)
# expect(response).to be_successful
# end
end

describe 'when logged out and p2p is disabled' do
before do
allow_any_instance_of(SystemSetting).to receive(:peer_to_peer?).and_return(false)
end

it 'redirects to the user login page for the index of contributions' do
create(:listing)
get contributions_url
assert_redirected_to new_user_session_url
end

it 'redirects to the user login page for a link to an individual contribution' do
contribution = create(:listing)
get contribution_url(contribution)
assert_redirected_to new_user_session_url
end
end

it 'allows asking for a specific subtype of listing' do
sign_in user
ask = create(:ask, title: 'this is the ask title')
offer = create(:offer, title: 'this is the offer title')
get contributions_url, params: { ContributionType: { 'Ask' => 1 } }
Expand All @@ -37,6 +89,7 @@
end

it 'parses requests for a filtered list' do
sign_in user
categories = [
create(:category, name: Faker::Lorem.word),
create(:category, name: Faker::Lorem.word)
Expand Down Expand Up @@ -64,16 +117,4 @@
expect(response_ids).not_to include(no_tags_correct_area_listing.id)
end
end

describe 'GET /contributions/:id' do
it 'is successful' do
contribution = create(:listing)

get(
contribution_url(contribution),
)

expect(response).to be_successful
end
end
end