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

Restructure new findaproject page for #4129 #4142

Open
wants to merge 10 commits into
base: development
Choose a base branch
from
14 changes: 7 additions & 7 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ group :development, :test do
gem 'pry-awesome_print'
gem 'pry-byebug'
gem 'rspec-rails'

# Profiling for use in prod
gem 'flamegraph'
gem 'memory_profiler'
gem 'meta_request'
gem 'rack-mini-profiler', require: false
gem 'stackprof'
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are dev tools so I moved it here. We can move it back in case it is being used in prod>

end

gem 'dotenv', group: [:development, :test], require: 'dotenv/load'
Expand All @@ -130,13 +137,6 @@ gem 'friendly_id'
gem 'iso-639'
gem 'rtl'

# Profiling for use in prod
gem 'flamegraph'
gem 'memory_profiler'
gem 'meta_request'
gem 'rack-mini-profiler'
gem 'stackprof'

gem 'bento-sdk', github: 'bentonow/bento-ruby-sdk', branch: 'master'
gem 'newrelic_rpm'

Expand Down
6 changes: 2 additions & 4 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,9 @@ def current_user
super || guest_user
end

#find the guest user account if a guest user session is currently active
# find the guest user account if a guest user session is currently active
def guest_user
unless session[:guest_user_id].nil?
User.where(id: session[:guest_user_id]).first
end
User.find_by(id: session[:guest_user_id])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exact same query in SQL, just shorter way of writing it. Will give nil if not present, so same function.

end

#when the user chooses to transcribe as guest, find guest user id or create new guest user
Expand Down
26 changes: 18 additions & 8 deletions app/controllers/dashboard_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,20 @@ def landing_page
def new_landing_page
@search_results = search_results(params[:search])

# Get random Collections and DocSets from paying users
@owners = User.findaproject_owners.order(:display_name).joins(:collections)
.left_outer_joins(:document_sets).includes(:collections)

# Sampled Randomly down to 8 items for Carousel
docsets = DocumentSet.carousel.includes(:owner).where(owner_user_id: @owners.ids.uniq).sample(5)
colls = Collection.carousel.includes(:owner).where(owner_user_id: @owners.ids.uniq).sample(5)
users = User.owners
.joins(:collections)
.left_outer_joins(:document_sets)
.includes({ random_collections: :tags }, { random_document_sets: { collection: :tags } })
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit tricky. So we cannot preload queries with dynamic like random.

So what I did here is remove the random_sample scope in random_collections, then we sample later via array to SQL calls.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although, in hindsight, this makes 'random_collection' and 'random_document_sets' scope feel like incorrect labels now.


@org_owners = users.findaproject_orgs.order(:display_name).distinct
@individual_owners = users.findaproject_individuals.order(:display_name).distinct
Copy link
Collaborator

Choose a reason for hiding this comment

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

These will be subqueries from users


docsets = DocumentSet.carousel
.includes(:owner, { next_untranscribed_page: :work })
.where(owner_user_id: @org_owners.select(:id) + @individual_owners.select(:id)).sample(5)
colls = Collection.carousel
.includes(:owner, { next_untranscribed_page: :work })
Copy link
Collaborator

Choose a reason for hiding this comment

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

N+1 issue here. Checking the html we access next_untranscribed_page, and then it's work. Both of these have to be preloaded.

Also it seems, as long as we preload correct relationships, turning it to array afterwards will not cause more query

.where(owner_user_id: @org_owners.select(:id) + @individual_owners.select(:id)).sample(5)
@collections = (docsets + colls).sample(8)

@tag_map = Tag.featured_tags.group(:ai_text).count
Expand Down Expand Up @@ -344,6 +351,9 @@ def send_generated_pdf(output_path)
def search_results(search_key)
return nil if search_key.nil?

Collection.search(search_key).unrestricted + DocumentSet.search(search_key).unrestricted
collections_query = Collection.search(search_key).unrestricted.includes(:owner)
document_sets_query = DocumentSet.search(search_key).unrestricted.includes(:owner)

collections_query + document_sets_query
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, we access owner from search_results so preload that too.

end
end
8 changes: 8 additions & 0 deletions app/controllers/user_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,14 @@ def profile

return unless @user.owner?

if params[:ai_text]
@tag = Tag.where(ai_text: params[:ai_text]).first
if @tag
tag_collections = @tag.collections.where(owner_user_id: @user.id)
tag_document_sets = tag_collections.map(&:document_sets).flatten
@tag_collections = tag_collections+tag_document_sets
end
end
collections = @user.all_owner_collections.carousel
sets = @user.document_sets.carousel
@carousel_collections = (collections + sets).sample(8)
Expand Down
5 changes: 4 additions & 1 deletion app/models/document_set.rb
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,14 @@ def text_and_metadata_entry?
self.collection.text_and_metadata_entry?
end


def hide_completed
self.collection.hide_completed
end

def tags
self.collection.tags
end

def review_workflow
self.collection.review_workflow
end
Expand Down
4 changes: 4 additions & 0 deletions app/models/tag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ class Tag < ApplicationRecord
require 'openai/description_tagger'
has_and_belongs_to_many :collections

# scope for only tags that are canonical
scope :canonical, -> { where(canonical: true) }


# return tags that are canonical and have collections that are unrestricted and have a picture and intro block
def self.featured_tags
joins(:collections).where(canonical: true).merge(Collection.unrestricted.has_intro_block.has_picture.not_empty)
Expand Down
6 changes: 4 additions & 2 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,16 +110,18 @@ class User < ApplicationRecord
has_many :notes, -> { order 'created_at DESC' }
has_many :deeds

has_many :random_collections, -> { unrestricted.has_intro_block.not_near_complete.not_empty.random_sample },
has_many :random_collections, -> { unrestricted.has_intro_block.not_near_complete.not_empty },
class_name: "Collection", :foreign_key => "owner_user_id"
has_many :random_document_sets, -> { unrestricted.has_intro_block.not_near_complete.not_empty.random_sample },
has_many :random_document_sets, -> { unrestricted.has_intro_block.not_near_complete.not_empty },
class_name: "DocumentSet", :foreign_key => "owner_user_id"

has_many :metadata_description_versions, :dependent => :destroy

scope :owners, -> { where(owner: true) }
scope :trial_owners, -> { owners.where(account_type: 'Trial') }
scope :findaproject_owners, -> { owners.where.not(account_type: [nil, 'Trial', 'Staff']) }
scope :findaproject_orgs, -> { owners.where(account_type: ['Large Organization', 'Small Organization']) }
scope :findaproject_individuals, -> { owners.where(account_type: ['Legacy', 'Individual Researcher']) }
scope :paid_owners, -> { non_trial_owners.where('paid_date > ?', Time.now) }
scope :expired_owners, -> { non_trial_owners.where('paid_date <= ?', Time.now) }
scope :active_mailers, -> { where(activity_email: true)}
Expand Down
9 changes: 9 additions & 0 deletions app/views/dashboard/_project_details.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-snippet = truncate(Loofah.fragment(project.intro_block).text(encode_special_chars: false), length: 300, separator: ' ') || ''
div.projects_details
-unless project.picture.blank?
.projects_details_image
=image_tag(project.picture_url(:thumb), alt: project.title)
.projects_collection
h5 =link_to project.title, collection_path(owner, project)
-unless snippet.empty?
.projects_collection_snippet = snippet
17 changes: 2 additions & 15 deletions app/views/dashboard/browse_tag.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,6 @@ h2

/ Display a list of collections with teaser images and snippets as tiles
[email protected] do |collection|
-snippet = to_snippet(collection.intro_block)
.collection-tile
.projects_details_image
=link_to(image_tag(collection.picture_url(:thumb), alt: collection.title), collection_path(collection.owner, collection), class: 'collection-tile_image')
.collection-tile_content
h3 =link_to collection.title, collection_path(collection.owner, collection)
p =snippet
-collection_stats(collection)
.collection-work_stats
span
=t('dashboard.owner.n_works', count: collection.works.count)
span
=render({ :partial => 'shared/progress', :locals => { :collection => collection } })


=render(partial: 'shared/collection_tile', locals: { collection: collection })


72 changes: 39 additions & 33 deletions app/views/dashboard/new_landing_page.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@

.columns.project-list
article.maincol
ul.tagcloud
-@tag_map.each do |ai_text, collection_count|
-if collection_count > 0
li data-weight="#{collection_count}"
=link_to(ai_text, browse_tag_path(ai_text))
-unless params[:search]
ul.tagcloud
-@tag_map.each do |ai_text, collection_count|
-if collection_count > 0
li data-weight="#{collection_count}"
=link_to(ai_text, browse_tag_path(ai_text))

-if @search_results && @owners.empty?
// show search results instead of owner listing
-if @search_results && @org_owners.empty? && @individual_owners.empty?
-@search_results.each do |sr|
.project-list_projects
.projects-owner
Expand All @@ -38,35 +40,39 @@
.projects_collection_snippet
=truncate(Loofah.fragment(sr.intro_block).text(encode_special_chars: false), length: 300, separator: ' ') || ''

[email protected] do |owner|
-if params[:search]
-projects = @search_results.select{ |p| p.owner_user_id == owner.id }
-else
-projects = (owner.random_collections + owner.random_document_sets).sample(3)
-unless projects.blank?
.project-list_projects
.projects-owner
h3.projects-owner_title
=link_to owner.display_name, user_profile_path(owner)
-if owner.about.present?
.description
=owner.about

.projects
-projects.each do |project|
-snippet = truncate(Loofah.fragment(project.intro_block).text(encode_special_chars: false), length: 300, separator: ' ') || ''
div.projects_details
-unless project.picture.blank?
// show listing of all organizations and individual owners
-else
-[@org_owners, @individual_owners].each do |owners|
-owners.each do |owner|
-projects = (owner.random_collections.sample(5) + owner.random_document_sets.sample(5))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We sample here as we can't preload with dynamic random query

-if projects.count > 0
.project-list_projects
.projects-owner
h3.projects-owner_title
=link_to owner.display_name, user_profile_path(owner)
-if owner.picture.present?
.projects_details_image
=image_tag(project.picture_url(:thumb), alt: project.title)
.projects_collection
h5 =link_to project.title, collection_path(owner, project)
-unless snippet.empty?
.projects_collection_snippet = snippet
.projects_link =link_to t('dashboard.landing_page.more'), user_profile_path(owner)
=image_tag(owner.picture_url(:thumb), alt: owner.display_name)
-else # find a thumbnail from their collections
/ display the most common tags for projects by this owner
-project = projects.first
-if project.picture.present?
.projects_details_image
=image_tag(project.picture_url(:thumb), alt: project.title)
-if owner.about.present?
.description
=owner.about
-tag_map = projects.flat_map(&:tags).group_by(&:canonical).map { |tag, tag_list| [tag.ai_text, tag_list.size] }.sort_by(&:last).reverse
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not 100% sure if this functions exactly how you intended it to be (so please correct me if I'm wrong here). Though based on my testing my approach produced less SQL queries (so no n+1 it seems). Not 100% sure

-if tag_map.present?
ul.tagcloud
-tag_map.each do |ai_text, collection_count|
-if collection_count > 0
li
-link_text = "#{ai_text} (#{collection_count})"
=link_to(link_text, tagged_user_profile_path(owner.slug, ai_text))

.project-list_link
=link_to t('.all_collections'), collections_list_path
.project-list_link
=link_to t('.all_collections'), collections_list_path

aside.sidecol
=form_tag({:controller => 'dashboard', :action => 'new_landing_page'}, method: :get, enforce_utf8: false, class: 'collection-search') do
Expand Down
14 changes: 14 additions & 0 deletions app/views/shared/_collection_tile.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
-snippet = to_snippet(collection.intro_block)
.collection-tile
.projects_details_image
=link_to(image_tag(collection.picture_url(:thumb), alt: collection.title), collection_path(collection.owner, collection), class: 'collection-tile_image')
.collection-tile_content
h3 =link_to collection.title, collection_path(collection.owner, collection)
p =snippet
-collection_stats(collection)
.collection-work_stats
span
=t('dashboard.owner.n_works', count: collection.works.count)
span
=render({ :partial => 'shared/progress', :locals => { :collection => collection } })

10 changes: 10 additions & 0 deletions app/views/user/_owner_profile.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ section.user-profile
h3 =link_to c.title, collection_path(c.owner, c)
p =snippet

-if @tag
h2
span Collections tagged
i [email protected]_text

/ Display a list of collections with teaser images and snippets as tiles
-@tag_collections.each do |collection|
= render partial: 'shared/collection_tile', locals: { collection: collection }


.columns
article.maincol
-if collections_and_document_sets.any?{ |c| c.class == Collection && c.supports_document_sets }
Expand Down
10 changes: 10 additions & 0 deletions config/initializers/rack_mini_profiler.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

if Rails.env.development?
require 'rack-mini-profiler'

Rack::MiniProfiler.config.position = 'bottom-right'
Rack::MiniProfiler.config.show_total_sql_count = true

Rack::MiniProfilerRails.initialize!(Rails.application)
end
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@


resources :document_sets, except: [:show, :create, :edit]
get '/:user_id/tagged/:ai_text', to: 'user#profile', as: :tagged_user_profile

scope ':user_slug' do
get 'update_profile', to: 'user#update_profile', as: :update_profile
Expand Down Expand Up @@ -511,7 +512,6 @@
get 'start_transcribing', as: :start_transcribing, to: 'collection#start_transcribing'



#work related routes
#have to use match because it must be both get and post
match ':work_id', to: 'display#read_work', via: [:get, :post], as: :read_work
Expand Down
Loading