Skip to content
Open
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
1 change: 1 addition & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class Application < Rails::Application
require "extends/controllers/decidim/participatory_processes/participatory_processes_controller_extends"
require "extends/controllers/decidim/budgets/projects_controller_extends"
require "extends/controllers/decidim/proposals/admin/proposal_states_controller_extends"
require "extends/controllers/decidim/proposals/proposals_controller_extends"
# helpers
require "extends/helpers/decidim/check_boxes_tree_helper_extends"
require "extends/helpers/decidim/omniauth_helper_extends"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# frozen_string_literal: true

require "active_support/concern"

module Decidim
module Proposals
module ProposalsControllerExtends
extend ActiveSupport::Concern

included do
def index
if component_settings.participatory_texts_enabled?
@proposals = Decidim::Proposals::Proposal
.where(component: current_component)
.published
.not_hidden
.only_amendables
.includes(:category, :scope, :attachments, :coauthorships)
.order(position: :asc)
render "decidim/proposals/proposals/participatory_texts/participatory_text"
else
@proposals = search.result
@proposals = reorder(@proposals)
ids = @proposals.ids
@proposals = Decidim::Proposals::Proposal.where(id: ids)
.order(Arel.sql("position(decidim_proposals_proposals.id::text in '#{ids.join(",")}')"))
Copy link
Contributor

Choose a reason for hiding this comment

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

String interpolation in Arel.sql() bypasses ActiveRecord's SQL injection protections.
Even though ids is safe here, this pattern could create risk if refactored in the future.

More importantly, I'm trying to understand the flow here:

L22 -> @proposals = search.result
L23 -> @proposals = reorder(@proposals)
L24 -> ids = @proposals.ids
L25-28 -> @proposals = Proposal.where(id: ids).order(Arel.sql(...))

Why do we execute the query to get ids (L24), then re-query the same proposals (L25-28) ?
Is this to preserve a specific ordering that reorder() doesn't handle ? Or could we
apply the ordering directly to the search.result relation ?

I might be missing something about how reorder() works here, I just want to be sure we're not making "useless requests" but the code seems good tho, well done !

.page(params[:page])
.per(per_page)
@proposals = @proposals.includes(:component, :coauthorships, :attachments)

@voted_proposals = if current_user
ProposalVote.where(
author: current_user,
proposal: @proposals.pluck(:id)
).pluck(:decidim_proposal_id)
else
[]
end
end
end
end
end
end
end

Decidim::Proposals::ProposalsController.include(Decidim::Proposals::ProposalsControllerExtends)
76 changes: 76 additions & 0 deletions spec/controllers/proposals_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# frozen_string_literal: true

require "spec_helper"

module Decidim
module Proposals
describe ProposalsController do
routes { Decidim::Proposals::Engine.routes }

let(:user) { create(:user, :confirmed, organization: component.organization) }

let(:proposal_params) do
{
component_id: component.id
}
end
let(:params) { { proposal: proposal_params } }

before do
request.env["decidim.current_organization"] = component.organization
request.env["decidim.current_participatory_space"] = component.participatory_space
request.env["decidim.current_component"] = component
stub_const("Decidim::Paginable::OPTIONS", [100])
end

describe "GET index" do
context "when participatory texts are disabled" do
let(:component) { create(:proposal_component, :with_geocoding_enabled) }

it "sorts proposals by search defaults" do
get :index
expect(response).to have_http_status(:ok)
expect(subject).to render_template(:index)
expect(assigns(:proposals).order_values).to eq(["position(decidim_proposals_proposals.id::text in '')"])
end

it "sets two different collections" do
geocoded_proposals = create_list(:proposal, 10, component:, latitude: 1.1, longitude: 2.2)
non_geocoded_proposals = create_list(:proposal, 2, component:, latitude: nil, longitude: nil)

get :index
expect(response).to have_http_status(:ok)
expect(subject).to render_template(:index)

expect(assigns(:proposals).count).to eq 12
expect(assigns(:proposals)).to match_array(geocoded_proposals + non_geocoded_proposals)
end
end

context "when participatory texts are enabled" do
let(:component) { create(:proposal_component, :with_participatory_texts_enabled) }

it "sorts proposals by position" do
get :index
expect(response).to have_http_status(:ok)
expect(subject).to render_template(:participatory_text)
expect(assigns(:proposals).order_values.first.expr.name).to eq("position")
end

context "when emendations exist" do
let!(:amendable) { create(:proposal, component:) }
let!(:emendation) { create(:proposal, component:) }
let!(:amendment) { create(:amendment, amendable:, emendation:, state: "accepted") }

it "does not include emendations" do
get :index
expect(response).to have_http_status(:ok)
emendations = assigns(:proposals).select(&:emendation?)
expect(emendations).to be_empty
end
end
end
end
end
end
end
Loading
Loading