diff --git a/app/assets/stylesheets/mo/_elements.scss b/app/assets/stylesheets/mo/_elements.scss index f2f41f4b27..22f3a9c1a7 100644 --- a/app/assets/stylesheets/mo/_elements.scss +++ b/app/assets/stylesheets/mo/_elements.scss @@ -4,6 +4,26 @@ body { word-wrap: break-word; } +// Give focus outlines breathing room so they don't clip text/icons. +a:focus, +button:focus, +.btn:focus, +[role="button"]:focus { + outline-offset: 2px; +} + +// Inside matrix boxes, overflow:hidden clips outlines. Use an +// inset box-shadow instead so the focus indicator is visible. +.rss-box-details { + a:focus, + button:focus, + .btn:focus, + [role="button"]:focus { + outline: none; + box-shadow: inset 0 0 0 2px rgba($link-color, 0.4); + } +} + blockquote { font-size: 100%; } diff --git a/app/assets/stylesheets/mo/_icons.scss b/app/assets/stylesheets/mo/_icons.scss index 5a8f3bb23d..9b5f7843a4 100644 --- a/app/assets/stylesheets/mo/_icons.scss +++ b/app/assets/stylesheets/mo/_icons.scss @@ -124,6 +124,8 @@ // This is for the stateful icon_link_to helper .icon-link, .panel-collapse-trigger { + text-decoration: none; + .active-icon, .active-label { display: none; } @@ -139,6 +141,13 @@ } } +// Bootstrap collapse sets display:block, which breaks +// table-row-group semantics on . Override so +// collapsed tbody rows render correctly when expanded. +tbody.collapse.in { + display: table-row-group; +} + // Fix for weird inherited indent from .name-section p .glyphicon { text-indent: 0; diff --git a/app/classes/checklist.rb b/app/classes/checklist.rb index 4ec9606aa6..a176088c20 100644 --- a/app/classes/checklist.rb +++ b/app/classes/checklist.rb @@ -44,15 +44,30 @@ def initialize(user) # Build list of species observed by one Project. class ForProject < Checklist - def initialize(project, location = nil) + def initialize(project, location = nil, + include_sub_locations: false) @project = project @location = location base = project.visible_observations - @observations = if location.present? - base.within_locations([location]) - else - base - end + @observations = + if location.present? && include_sub_locations + sub_location_observations(base, location) + elsif location.present? + base.within_locations([location]) + else + base + end + end + + def sub_location_observations(base, location) + escaped = ActiveRecord::Base.sanitize_sql_like( + location.name + ) + tbl = Location.arel_table + base.joins(:location).where( + tbl[:name].matches("%, #{escaped}"). + or(tbl[:name].eq(location.name)) + ) end delegate :target_name_ids, to: :@project diff --git a/app/components/projects/locations_table.rb b/app/components/projects/locations_table.rb index 33f10f7a51..8629fcc7c9 100644 --- a/app/components/projects/locations_table.rb +++ b/app/components/projects/locations_table.rb @@ -2,37 +2,168 @@ module Components module Projects - # Renders the project locations table with aliases and target - # location remove buttons. + # Renders the project locations table with target location + # grouping, collapsible sub-locations, and aliases. class LocationsTable < Components::Base - def initialize(project:, locations:, user: nil) + def initialize(project:, grouped_data:, + ungrouped_locations:, obs_counts:, + user: nil) super() @project = project - @locations = locations + @grouped_data = grouped_data + @ungrouped_locations = ungrouped_locations + @obs_counts = obs_counts @user = user end def view_template div(id: "locations_table") do - table(class: "table table-striped " \ - "table-project-members mt-3") do - thead { render_header } - tbody do - @locations.each { |loc| render_row(loc) } - end - end + render_target_groups if @grouped_data.any? + render_ungrouped if @ungrouped_locations.any? end end private def admin? - @project.is_admin?(@user) + return @admin if defined?(@admin) + + @admin = @project.is_admin?(@user) + end + + # --- Target location groups (collapsible) --- + + def render_target_groups + table(class: "table table-striped " \ + "table-project-members mt-3") do + thead { render_header } + @grouped_data.each do |group| + render_target_group(group) + end + end + end + + def render_target_group(group) + target = group[:target] + subs = group[:sub_locations] + collapse_id = "target_subs_#{target.id}" + count = target_obs_count(target, subs) + + render_target_row(target, collapse_id, count, subs) + render_sub_location_rows(subs, collapse_id) + end + + def render_target_row(target, collapse_id, count, subs) + tbody do + tr do + render_target_name_cell(target, collapse_id, subs) + td(class: "align-middle") { plain(count.to_s) } + render_aliases_cell(target) + render_target_column(target) if admin? + end + end + end + + def render_target_name_cell(target, collapse_id, subs) + td(class: "align-middle") do + render_chevron(collapse_id) if subs.any? + plain(" ") if subs.any? + link_to( + target.display_name, + checklist_path(project_id: @project.id, + location_id: target.id, + sub_locations: 1) + ) + end + end + + def render_sub_location_rows(subs, collapse_id) + return if subs.empty? + + tbody(id: collapse_id, class: "collapse") do + subs.each { |loc| render_sub_row(loc) } + end + end + + def render_sub_row(loc) + render_location_row(loc, indent: true) + end + + def render_chevron(collapse_id) + link_to( + "javascript:void(0)", + role: :button, + class: "panel-collapse-trigger collapsed", + data: { toggle: "collapse", + target: "##{collapse_id}" }, + aria: { expanded: false, + controls: collapse_id } + ) do + link_icon(:chevron_down, title: :OPEN.l, + class: "active-icon") + link_icon(:chevron_up, title: :CLOSE.l) + end + end + + def target_obs_count(target, subs) + count = @obs_counts[target.id] || 0 + subs.each { |loc| count += @obs_counts[loc.id] || 0 } + count + end + + # --- Ungrouped locations (flat table) --- + + def render_ungrouped + table(class: "table table-striped " \ + "table-project-members mt-3") do + thead { render_header } + tbody do + @ungrouped_locations.each do |loc| + render_ungrouped_row(loc) + end + end + end + end + + def render_ungrouped_row(loc) + render_location_row(loc) + end + + # --- Shared --- + + def render_location_row(loc, indent: false) + count = @obs_counts[loc.id] || 0 + tr do + render_location_name_cell(loc, indent: indent) + td(class: "align-middle") { plain(count.to_s) } + render_aliases_cell(loc) + td { nil } if admin? + end + end + + def render_location_name_cell(loc, indent: false) + style = indent ? "padding-left: 2em" : nil + td(class: "align-middle", style: style) do + link_to( + loc.display_name, + checklist_path(project_id: @project.id, + location_id: loc.id) + ) + end + end + + def render_aliases_cell(loc) + td(class: "align-middle") do + render(Components::ProjectAliases.new( + project: @project, target: loc + )) + end end def render_header tr do th { :LOCATION.t } + th { :OBSERVATIONS.t } th { :PROJECT_ALIASES.t } if admin? th(class: "text-center") do @@ -42,24 +173,6 @@ def render_header end end - def render_row(loc) - tr do - td(class: "align-middle") do - link_to( - loc.display_name, - checklist_path(project_id: @project.id, - location_id: loc.id) - ) - end - td(class: "align-middle") do - render(Components::ProjectAliases.new( - project: @project, target: loc - )) - end - render_target_column(loc) if admin? - end - end - def render_target_column(loc) td(class: "align-middle text-center") do render_remove_button(loc) if target?(loc) diff --git a/app/controllers/checklists_controller.rb b/app/controllers/checklists_controller.rb index 3cf2daf3df..a5e3444971 100644 --- a/app/controllers/checklists_controller.rb +++ b/app/controllers/checklists_controller.rb @@ -41,8 +41,10 @@ def project_checklist(proj_id, location_id) return unless (@project = find_or_goto_index(Project, proj_id)) @location = Location.safe_find(location_id) + sub = params[:sub_locations] == "1" - Checklist::ForProject.new(@project, @location) + Checklist::ForProject.new(@project, @location, + include_sub_locations: sub) end def species_list_checklist(list_id) diff --git a/app/controllers/concerns/projects/location_grouping.rb b/app/controllers/concerns/projects/location_grouping.rb new file mode 100644 index 0000000000..e80e777b89 --- /dev/null +++ b/app/controllers/concerns/projects/location_grouping.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module Projects + # Shared logic for grouping project locations under target + # locations by name suffix. + module LocationGrouping + private + + def build_grouped_locations(project) + obs_locs = project.locations.distinct.to_a + targets = sorted_targets(project) + sorted_obs = obs_locs.sort_by(&:scientific_name) + return [[], sorted_obs] if targets.empty? + + groups = build_groups(obs_locs, targets) + grouped_ids = collect_grouped_ids(groups, targets) + ungrouped = sorted_obs.reject do |l| + grouped_ids.include?(l.id) + end + [groups, ungrouped] + end + + def sorted_targets(project) + project.target_locations. + order(:scientific_name).to_a + end + + def build_groups(obs_locs, targets) + assignments = assign_to_targets(obs_locs, targets) + targets.map do |target| + subs = (assignments[target.id] || []). + sort_by(&:scientific_name) + { target: target, sub_locations: subs } + end + end + + # Assign each observed location to its most specific + # (longest name) matching target to avoid duplicates. + def assign_to_targets(obs_locs, targets) + assignments = {} + obs_locs.each do |loc| + best = most_specific_target(loc, targets) + next unless best + + (assignments[best.id] ||= []) << loc + end + assignments + end + + def most_specific_target(loc, targets) + matches = targets.select do |t| + loc.id != t.id && loc.name.end_with?(", #{t.name}") + end + matches.max_by { |t| t.name.length } + end + + def collect_grouped_ids(groups, target_locs) + ids = Set.new(target_locs.map(&:id)) + groups.each do |g| + g[:sub_locations].each { |loc| ids.add(loc.id) } + end + ids + end + + def observation_counts(project) + project.visible_observations. + where.not(location_id: nil). + group(:location_id).count + end + end +end diff --git a/app/controllers/projects/locations_controller.rb b/app/controllers/projects/locations_controller.rb index 313416d467..792084b920 100644 --- a/app/controllers/projects/locations_controller.rb +++ b/app/controllers/projects/locations_controller.rb @@ -2,31 +2,24 @@ module Projects class LocationsController < ApplicationController + include Projects::LocationGrouping + before_action :login_required def index return unless find_project! - @locations = merged_locations + @grouped_data, @ungrouped_locations = + build_grouped_locations(@project) + @obs_counts = observation_counts(@project) end private def find_project! - @project = find_or_goto_index(Project, params[:project_id].to_s) - end - - # Merge observation-derived locations with target locations, - # removing duplicates. - def merged_locations - obs_locs = @project.locations.distinct - target_locs = @project.target_locations - all_locs = (obs_locs.to_a + target_locs.to_a).uniq(&:id) - sort_locations(all_locs) - end - - def sort_locations(locs) - locs.sort_by(&:scientific_name) + @project = find_or_goto_index( + Project, params[:project_id].to_s + ) end end end diff --git a/app/controllers/projects/target_locations_controller.rb b/app/controllers/projects/target_locations_controller.rb index 0259ae5fb3..a2ae486c53 100644 --- a/app/controllers/projects/target_locations_controller.rb +++ b/app/controllers/projects/target_locations_controller.rb @@ -2,6 +2,8 @@ module Projects class TargetLocationsController < ApplicationController + include Projects::LocationGrouping + before_action :login_required before_action :set_project before_action :require_admin @@ -40,7 +42,9 @@ def destroy private def set_project - @project = find_or_goto_index(Project, params[:project_id]) + @project = find_or_goto_index( + Project, params[:project_id] + ) end def require_admin @@ -51,7 +55,9 @@ def require_admin end def redirect_to_locations - redirect_to(project_locations_path(project_id: @project.id)) + redirect_to( + project_locations_path(project_id: @project.id) + ) end def parse_locations_from_params @@ -81,19 +87,15 @@ def add_locations(locations) end def render_locations_update - locations = merged_locations + grouped, ungrouped = build_grouped_locations(@project) + counts = observation_counts(@project) render( partial: "projects/target_locations/locations_update", locals: { project: @project, user: @user, - locations: locations } + grouped_data: grouped, + ungrouped_locations: ungrouped, + obs_counts: counts } ) end - - def merged_locations - obs_locs = @project.locations.distinct.to_a - target_locs = @project.target_locations.to_a - all_locs = (obs_locs + target_locs).uniq(&:id) - all_locs.sort_by(&:scientific_name) - end end end diff --git a/app/models/location/scopes.rb b/app/models/location/scopes.rb index bd0da3f2fe..090c26c3eb 100644 --- a/app/models/location/scopes.rb +++ b/app/models/location/scopes.rb @@ -29,6 +29,14 @@ module Location::Scopes where(Location[:name].matches("%#{region}")) end } + # Locations whose name ends with ", " + # (i.e. sub-locations within a parent region by name hierarchy) + scope :sub_locations_of, lambda { |target_location| + escaped = sanitize_sql_like(target_location.name) + where(Location[:name].matches("%, #{escaped}")). + where.not(id: target_location.id) + } + scope :name_has, ->(phrase) { search_columns(Location[:name], phrase) } # Used by Lookup::Locations diff --git a/app/models/project.rb b/app/models/project.rb index dedd49de12..f98ae8f4bf 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -453,7 +453,20 @@ def candidate_name_ids def candidate_location_ids return unless target_locations.any? - Observation.within_locations(target_locations).select(:id) + scope = Observation.joins(:location). + where(location_suffix_conditions) + scope = scope.merge(Observation.in_box(**location.bounding_box)) if location + scope.select("observations.id") + end + + # OR clause: location.name LIKE '%, ' OR = '' + def location_suffix_conditions + tbl = Location.arel_table + target_locations.map do |tl| + escaped = self.class.sanitize_sql_like(tl.name) + tbl[:name].matches("%, #{escaped}"). + or(tbl[:name].eq(tl.name)) + end.reduce(:or) end public diff --git a/app/views/controllers/projects/locations/index.html.erb b/app/views/controllers/projects/locations/index.html.erb index ca7aa0ddde..6dcce109e8 100644 --- a/app/views/controllers/projects/locations/index.html.erb +++ b/app/views/controllers/projects/locations/index.html.erb @@ -9,5 +9,9 @@ add_project_banner(@project) <% end %> <%= render(Components::Projects::LocationsTable.new( - project: @project, locations: @locations, user: @user + project: @project, + grouped_data: @grouped_data, + ungrouped_locations: @ungrouped_locations, + obs_counts: @obs_counts, + user: @user )) %> diff --git a/app/views/controllers/projects/target_locations/_locations_update.erb b/app/views/controllers/projects/target_locations/_locations_update.erb index b3f5117e65..e592d6acf7 100644 --- a/app/views/controllers/projects/target_locations/_locations_update.erb +++ b/app/views/controllers/projects/target_locations/_locations_update.erb @@ -4,7 +4,11 @@ <% end %> <%= turbo_stream.replace("locations_table") do %> <%= render(Components::Projects::LocationsTable.new( - project: project, locations: locations, user: user + project: project, + grouped_data: grouped_data, + ungrouped_locations: ungrouped_locations, + obs_counts: obs_counts, + user: user )) %> <% end %> <%= turbo_stream.replace("project_tabs") do %> diff --git a/test/components/projects/locations_table_test.rb b/test/components/projects/locations_table_test.rb index b8101cffc1..1f0656b04c 100644 --- a/test/components/projects/locations_table_test.rb +++ b/test/components/projects/locations_table_test.rb @@ -32,10 +32,11 @@ def test_non_admin_sees_no_target_column end def test_non_target_location_has_no_remove_button - # Add albion as a non-target observed location - project.add_observation(observations(:minimal_unknown_obs)) - locs = [locations(:burbank), locations(:albion)] - html = render_table(user: users(:rolf), locations: locs) + # Albion is a non-target observed location + html = render_table( + user: users(:rolf), + ungrouped: [locations(:albion)] + ) # Burbank (target) should have remove button burbank_id = locations(:burbank).id @@ -51,16 +52,51 @@ def test_non_target_location_has_no_remove_button ) end + def test_chevron_shown_for_target_with_sub_locations + burbank = locations(:burbank) + albion = locations(:albion) + grouped = [{ target: burbank, sub_locations: [albion] }] + html = render_table( + user: users(:rolf), + grouped: grouped, + obs_counts: { burbank.id => 2, albion.id => 3 } + ) + + # Chevron trigger present + assert_html(html, ".panel-collapse-trigger") + # Collapse target for sub-locations + assert_html(html, "#target_subs_#{burbank.id}") + # Aggregated count: 2 + 3 = 5 + assert_includes(html, "5") + end + + def test_no_chevron_for_target_without_sub_locations + html = render_table(user: users(:rolf)) + + assert_no_html(html, ".panel-collapse-trigger") + end + private def project projects(:rare_fungi_project) end - def render_table(user:, locations: [locations(:burbank)]) + def render_table(user:, grouped: nil, ungrouped: [], + obs_counts: {}) + grouped ||= default_grouped_data render(Components::Projects::LocationsTable.new( - project: project, locations: locations, user: user + project: project, + grouped_data: grouped, + ungrouped_locations: ungrouped, + obs_counts: obs_counts, + user: user )) end + + def default_grouped_data + burbank = locations(:burbank) + [{ target: burbank, sub_locations: [] }] + end end end diff --git a/test/controllers/projects/locations_controller_test.rb b/test/controllers/projects/locations_controller_test.rb index 02f764331b..72c53f8224 100644 --- a/test/controllers/projects/locations_controller_test.rb +++ b/test/controllers/projects/locations_controller_test.rb @@ -23,5 +23,59 @@ def test_index_scientific assert_match(loc.display_name, @response.body) assert_response(:success) end + + def test_index_with_target_locations + project = projects(:rare_fungi_project) + login + get(:index, params: { project_id: project.id }) + + assert_response(:success) + target = locations(:burbank) + # Target location name should appear even with no obs + assert_match(target.display_name, @response.body) + end + + def test_index_without_target_locations + project = projects(:eol_project) + login + get(:index, params: { project_id: project.id }) + + assert_response(:success) + # No collapse elements when there are no targets + assert_no_match(/panel-collapse-trigger/, @response.body) + end + + # Exercises grouping logic: assign_to_targets, + # most_specific_target, and ungrouped filtering + def test_index_groups_sub_locations_under_targets + project = projects(:rare_fungi_project) + california = locations(:california) + albion = locations(:albion) # "Albion, California, USA" + nybg = locations(:nybg_location) # New York — not a sub + + # Add California as a target location + project.add_target_location(california) + + # Add observations at Albion (sub of California) and NYBG + [albion, nybg].each do |loc| + obs = Observation.create!( + name: names(:fungi), user: users(:rolf), + location: loc, when: Time.zone.now + ) + project.observations << obs + end + + login + get(:index, params: { project_id: project.id }) + assert_response(:success) + + body = @response.body + # California target should appear + assert_match(california.display_name, body) + # Albion grouped under California as a sub-location + assert_match(albion.display_name, body) + # NYBG appears ungrouped (not a sub of any target) + assert_match(nybg.display_name, body) + end end end diff --git a/test/models/checklist_test.rb b/test/models/checklist_test.rb index 3fb12c1c9e..101c2bbc53 100644 --- a/test/models/checklist_test.rb +++ b/test/models/checklist_test.rb @@ -216,6 +216,70 @@ def test_checklist_for_project_target_names_with_observations assert_includes(taxa_names, "Agaricus campestris") end + def test_checklist_for_project_include_sub_locations + proj = projects(:bolete_project) + california = locations(:california) + albion = locations(:albion) # "Albion, California, USA" + + # Add observation in Albion (a sub-location of California) + obs = Observation.create!( + name: names(:coprinus_comatus), + user: mary, + location: albion, + when: Time.zone.now + ) + proj.observations << obs + + # With include_sub_locations: name suffix match + data_with = Checklist::ForProject.new( + proj, california, include_sub_locations: true + ) + + assert_operator( + data_with.num_taxa, :>=, 1, + "Sub-location obs should appear with include_sub_locations" + ) + taxa_names = data_with.taxa.pluck(0) + assert_includes(taxa_names, "Coprinus comatus") + end + + # Verify suffix matching excludes GPS-overlap observations + # (the bug that #4126 fixes: e.g., Ohio obs inside WV box) + def test_checklist_sub_locations_excludes_gps_overlap + proj = projects(:bolete_project) + california = locations(:california) + + # Create a non-California location with GPS inside CA box + nevada_loc = Location.create!( + name: "Reno, Nevada, USA", + scientific_name: "USA, Nevada, Reno", + north: 39.6, south: 39.4, east: -119.7, west: -119.9, + user: mary + ) + overlap_obs = Observation.create!( + name: names(:boletus_edulis), + user: mary, location: nevada_loc, + lat: 39.5, lng: -119.8, when: Time.zone.now + ) + proj.observations << overlap_obs + + # Without sub_locations (GPS bounding box): should include it + data_gps = Checklist::ForProject.new(proj, california) + assert_includes( + data_gps.taxa.pluck(0), "Boletus edulis", + "GPS-box match should include obs inside CA bounding box" + ) + + # With sub_locations (name suffix): should exclude it + data_suffix = Checklist::ForProject.new( + proj, california, include_sub_locations: true + ) + assert_not_includes( + data_suffix.taxa.pluck(0), "Boletus edulis", + "Suffix match should exclude Nevada obs despite GPS overlap" + ) + end + def test_checklist_for_species_lists list = species_lists(:unknown_species_list) data = Checklist::ForSpeciesList.new(list) diff --git a/test/models/location_test.rb b/test/models/location_test.rb index 2dfc254156..3bb3888bff 100644 --- a/test/models/location_test.rb +++ b/test/models/location_test.rb @@ -620,6 +620,19 @@ def north_southerthan_south_box { north: cal.south - 10, south: cal.south, east: cal.east, west: cal.west } end + def test_scope_sub_locations_of + california = locations(:california) + subs = Location.sub_locations_of(california) + + # Albion, Burbank, etc. end with ", California, USA" + assert_includes(subs, locations(:albion)) + assert_includes(subs, locations(:burbank)) + # California itself is excluded + assert_not_includes(subs, california) + # Unrelated locations excluded + assert_not_includes(subs, locations(:nybg_location)) + end + # supplements API tests def test_scope_in_box cal = locations(:california)