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

Optimize the proposals loading performance #39

Draft
wants to merge 4 commits into
base: release/0.28-stable-bcn
Choose a base branch
from
Draft
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
3 changes: 0 additions & 3 deletions decidim-core/app/cells/decidim/activity_cell.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,6 @@ def cache_hash
hash << I18n.locale.to_s
hash << model.class.name.underscore
hash << model.cache_key_with_version
if (author_cell = author)
hash.push(Digest::MD5.hexdigest(author_cell.send(:cache_hash)))
end

hash.join(Decidim.cache_key_separator)
end
Expand Down
8 changes: 4 additions & 4 deletions decidim-core/app/cells/decidim/author/show.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% data = has_tooltip? ? { tooltip: render(:profile_minicard).html_safe } : nil %>
<span class="author" data-author>
<%= content_tag :span, class: "author__container#{" is-compact" if layout == :compact}", data: do %>
<% tooltip = options.has_key?(:demo) ? { tooltip: render(:profile_minicard).html_safe } : nil %>
<%= content_tag(:span, class: :author, data: ) do %>
<%= content_tag :span, class: "author__container#{" is-compact" if layout == :compact}", data: tooltip do %>
<% if layout == :compact %>
<%= render :avatar %>

Expand All @@ -24,4 +24,4 @@
<%= render action %>
<% end %>
<% end %>
</span>
<% end %>
24 changes: 24 additions & 0 deletions decidim-core/app/cells/decidim/author_cell.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,24 @@ def context_actions_options
@context_actions_options ||= options[:context_actions].map(&:to_sym)
end

def profile_minicard
render
end

private

def data
@data ||= begin
internal_data = { author: true }
if has_tooltip? && !options.has_key?(:demo)
internal_data["remote_tooltip"] = true
internal_data["tooltip-url"] = decidim.profile_tooltip_path(raw_model.nickname)
end

internal_data
end
end

def layout
@layout ||= LAYOUTS.include?(options[:layout]) ? options[:layout] : :default
end
Expand Down Expand Up @@ -162,5 +178,13 @@ def resource_i18n_scope
def resource_name
@resource_name ||= from_context.class.name.demodulize.underscore
end

def has_tooltip?
return false if model.deleted?
return false if model.respond_to?(:blocked?) && model.blocked?
return true if options.has_key?(:tooltip)

model.has_tooltip?
end
end
end
4 changes: 4 additions & 0 deletions decidim-core/app/controllers/decidim/profiles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ def show
redirect_to profile_activity_path(nickname: params[:nickname])
end

def tooltip
render json: { data: cell("decidim/author", profile_holder.presenter).profile_minicard }
end

def following
@content_cell = "decidim/following"
@title_key = "following"
Expand Down
3 changes: 3 additions & 0 deletions decidim-core/app/packs/src/decidim/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import markAsReadNotifications from "src/decidim/notifications"
import RemoteModal from "src/decidim/remote_modal"
import selectActiveIdentity from "src/decidim/identity_selector_dialog"
import createTooltip from "src/decidim/tooltips"
import fetchRemoteTooltip from "src/decidim/remote_tooltips"
import createToggle from "src/decidim/toggle"
import {
createAccordion,
Expand Down Expand Up @@ -189,6 +190,8 @@ const initializer = (element = document) => {
// Initialize data-toggles
element.querySelectorAll("[data-toggle]").forEach((elem) => createToggle(elem))

element.querySelectorAll("[data-remote-tooltip]").forEach((elem) => fetchRemoteTooltip(elem))

element.querySelectorAll(".new_report").forEach((elem) => changeReportFormBehavior(elem))
}

Expand Down
31 changes: 31 additions & 0 deletions decidim-core/app/packs/src/decidim/remote_tooltips.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import createTooltip from "src/decidim/tooltips"

/**
* This function is parsing the DOM and tries to attach and initialize a tooltip starting from an element
* containing the following structure:
* <span data-remote-tooltip="true" tooltip-url="some url" data-author="true">
* <span></span>
* </span>
*
* @param {HTMLElement} node The element holding the initialization data
* @returns {void}
*/
export default async function(node) {
const container = node.firstElementChild;

if (container) {
const response = await fetch(node.dataset.tooltipUrl, {
headers: {
"Content-Type": "application/json"
}
});
if (response.ok) {
const json = await response.json();

container.dataset.tooltip = json.data;
createTooltip(container);
} else {
console.error(response.status, response.statusText);
}
}
}
1 change: 1 addition & 0 deletions decidim-core/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@
get "group_members", to: "profiles#group_members", as: "profile_group_members"
get "group_admins", to: "profiles#group_admins", as: "profile_group_admins"
get "activity", to: "user_activities#index", as: "profile_activity"
get "tooltip", to: "profiles#tooltip", as: "profile_tooltip"
resources :conversations, except: [:destroy], controller: "user_conversations", as: "profile_conversations"
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@
end

context "when the user has verified organizations" do
let(:user_group) { create(:user_group, :verified) }
let(:user_group) { create(:user_group, :verified, organization:) }
let(:content) { "This is a new comment" }

before do
Expand Down Expand Up @@ -1025,7 +1025,6 @@
end

context "when authenticated" do
let!(:organization) { create(:organization) }
let!(:user) { create(:user, :confirmed, organization:) }
let!(:comments) { create_list(:comment, 3, commentable:) }

Expand Down
20 changes: 0 additions & 20 deletions decidim-core/spec/cells/decidim/activity_cell_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,26 +87,6 @@
allow(controller).to receive(:current_user).and_return(nil)
end

context "when the author is shown" do
let(:show_author) { true }

context "and the user is updated" do
let!(:original_hash) { subject.send(:cache_hash) }

before do
# rubocop:disable Rails/SkipsModelValidations
resource.normalized_author.touch
# rubocop:enable Rails/SkipsModelValidations

subject.user.reload
end

it "changes the cache hash" do
expect(subject.send(:cache_hash)).not_to eq(original_hash)
end
end
end

context "when the author is hidden" do
context "and the user is updated" do
let!(:original_hash) { subject.send(:cache_hash) }
Expand Down
3 changes: 2 additions & 1 deletion decidim-core/spec/system/last_activity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,14 @@
organization:)
end
let(:long_body_comment) { "This is my very long comment for Last Activity card that must be shorten up because is more than 100 chars" }
let(:another_comment) { create(:comment, body: long_body_comment) }
let(:another_comment) { create(:comment, body: long_body_comment, commentable: second_commentable) }
let(:component) do
create(:component, :published, organization:)
end
let(:resource) do
create(:dummy_resource, component:, published_at: Time.current)
end
let(:second_commentable) { create(:dummy_resource, component:) }

before do
allow(Decidim::ActionLog).to receive(:public_resource_types).and_return(
Expand Down
2 changes: 1 addition & 1 deletion decidim-core/spec/system/user_activity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

describe "User activity" do
let(:organization) { create(:organization) }
let(:comment) { create(:comment) }
let(:comment) { create(:comment, commentable: resource) }
let(:user) { create(:user, :confirmed, organization:) }

let!(:action_log) do
Expand Down
35 changes: 17 additions & 18 deletions decidim-proposals/app/cells/decidim/proposals/proposal_l_cell.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,24 @@ def metadata_cell
end

def cache_hash
hash = []
hash << I18n.locale.to_s
hash << model.cache_key_with_version
hash << model.proposal_votes_count
hash << model.endorsements_count
hash << model.comments_count
hash << Digest::MD5.hexdigest(model.component.cache_key_with_version)
hash << Digest::MD5.hexdigest(resource_image_url) if resource_image_url
hash << render_space? ? 1 : 0
if current_user
hash << current_user.cache_key_with_version
hash << current_user.follows?(model) ? 1 : 0
@cache_hash ||= begin
hash = []
hash << I18n.locale.to_s
hash << self.class.name.demodulize.underscore
hash << model.cache_key_with_version
hash << model.proposal_votes_count
hash << model.endorsements_count
hash << model.comments_count
hash << Digest::MD5.hexdigest(model.component.cache_key_with_version)
hash << Digest::MD5.hexdigest(resource_image_url) if resource_image_url
hash << render_space? ? 1 : 0
hash << model.follows_count
hash << Digest::MD5.hexdigest(model.authors.map(&:cache_key_with_version).to_s)
hash << (model.must_render_translation?(model.organization) ? 1 : 0) if model.respond_to?(:must_render_translation?)
hash << model.component.participatory_space.active_step.id if model.component.participatory_space.try(:active_step)

hash.join(Decidim.cache_key_separator)
end
hash << model.follows_count
hash << Digest::MD5.hexdigest(model.authors.map(&:cache_key_with_version).to_s)
hash << (model.must_render_translation?(model.organization) ? 1 : 0) if model.respond_to?(:must_render_translation?)
hash << model.component.participatory_space.active_step.id if model.component.participatory_space.try(:active_step)

hash.join(Decidim.cache_key_separator)
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@ def index
.order(position: :asc)
render "decidim/proposals/proposals/participatory_texts/participatory_text"
else
@base_query = search
.result
.published
.not_hidden
@proposals = search.result

@proposals = @base_query.includes(:component, :coauthorships, :attachments)
@all_geocoded_proposals = @base_query.geocoded
@all_geocoded_proposals = @proposals.geocoded

@proposals = reorder(@proposals)
@proposals = paginate(@proposals)
@proposals = @proposals.includes(:component, :coauthorships, :attachments)

@voted_proposals = if current_user
ProposalVote.where(
Expand All @@ -59,8 +59,6 @@ def index
else
[]
end
@proposals = reorder(@proposals)
@proposals = paginate(@proposals)
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ module Decidim::Proposals
old_hash = my_cell.send(:cache_hash)
allow(I18n).to receive(:locale).and_return(alt_locale)

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -94,6 +95,7 @@ module Decidim::Proposals
old_hash = my_cell.send(:cache_hash)
proposal.update!(title: { en: "New title" })

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -103,6 +105,7 @@ module Decidim::Proposals
old_hash = my_cell.send(:cache_hash)
create(:endorsement, resource: proposal, author: build(:user, organization: proposal.participatory_space.organization))

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -113,6 +116,7 @@ module Decidim::Proposals
create(:proposal_vote, proposal:)
my_cell.model.reload

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -124,28 +128,20 @@ module Decidim::Proposals
component.settings = { foo: "bar" }
component.save!

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)

component.settings = component_settings
end
end

context "when no current user" do
it "generate a different hash" do
old_hash = my_cell.send(:cache_hash)
allow(controller).to receive(:current_user).and_return(nil)

expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end

context "when followers changes" do
let(:another_user) { create(:user, organization: proposal.participatory_space.organization) }

it "generate a different hash" do
old_hash = my_cell.send(:cache_hash)
create(:follow, followable: proposal, user: another_user)

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -155,6 +151,7 @@ module Decidim::Proposals
old_hash = my_cell.send(:cache_hash)
create(:follow, followable: proposal, user:)

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -165,6 +162,7 @@ module Decidim::Proposals
old_hash = my_cell.send(:cache_hash)
model.add_coauthor(user)

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end

Expand All @@ -173,6 +171,7 @@ module Decidim::Proposals
old_hash = my_cell.send(:cache_hash)
model.authors.first.update(personal_url: "new personal url")

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -196,6 +195,7 @@ module Decidim::Proposals
old_hash = my_cell.send(:cache_hash)
my_cell.context.merge!({ show_space: true })

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand Down Expand Up @@ -226,6 +226,7 @@ module Decidim::Proposals
step2.update!(active: true)
proposal.reload

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand All @@ -241,6 +242,7 @@ module Decidim::Proposals
step3.update!(active: true)
proposal.reload

my_cell.remove_instance_variable(:@cache_hash)
expect(my_cell.send(:cache_hash)).not_to eq(old_hash)
end
end
Expand Down
Loading