From c12835bf96fc50e7d13307f6317be55e3a6627ee Mon Sep 17 00:00:00 2001 From: Alex Date: Sat, 26 Sep 2026 01:27:27 +0800 Subject: [PATCH] feat: follow up fixes redesign --- CONTEXT.md | 8 + app/controllers/courses_controller.rb | 59 +++++- app/controllers/homescreen_controller.rb | 10 +- app/models/user.rb | 8 + app/views/courses/_groups_tab.html.erb | 6 +- app/views/courses/_groups_table.html.erb | 20 +- app/views/courses/_people_tab.html.erb | 4 +- app/views/courses/_students_section.html.erb | 6 +- app/views/courses/_students_table.html.erb | 13 +- .../courses/_topic_directory_tab.html.erb | 4 +- .../_topics_by_supervisor_list.html.erb | 29 +-- app/views/courses/_topics_section.html.erb | 7 +- app/views/shared/_list_item.html.erb | 44 +++- app/views/shared/_sidebar.html.erb | 2 +- app/views/shared/_table_empty_state.html.erb | 16 ++ app/views/styleguide/show.html.erb | 7 + app/views/topics/_copy_topic_details.html.erb | 195 +++++++++--------- .../topics/_copy_topic_list_item.html.erb | 33 +++ app/views/topics/_copy_topic_overlay.html.erb | 119 +++++------ app/views/topics/_template_fields.html.erb | 5 +- ...py-topic-list-item-reuses-shared-locals.md | 63 ++++++ ...8-empty-state-decided-in-the-controller.md | 108 ++++++++++ test/controllers/courses_controller_test.rb | 127 ++++++++++++ .../homescreen_cards_render_test.rb | 21 ++ test/system/topics/copy_topic_dialog_test.rb | 78 ++++++- 25 files changed, 759 insertions(+), 233 deletions(-) create mode 100644 app/views/shared/_table_empty_state.html.erb create mode 100644 app/views/topics/_copy_topic_list_item.html.erb create mode 100644 docs/adr/0017-copy-topic-list-item-reuses-shared-locals.md create mode 100644 docs/adr/0018-empty-state-decided-in-the-controller.md diff --git a/CONTEXT.md b/CONTEXT.md index 37e1fe9e..42a948c6 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -61,6 +61,14 @@ definitions. - **pinned (You) group** — the current viewer's own supervisor group in the topics directory, always first with a gray "(You)" suffix. Staff (lecturer/coordinator) only, pinned even at 0 topics; replaces the old "My Topics" section. Students browsing see plain A→Z with no pinned group. - **available badge** — on topics-directory rows only, an approved-and-unclaimed topic (`Topic#available?` = `approved? && proposed_project_instances.none?`) renders its pill as green "Available" *in place of* the "Approved" status pill (same approved-green palette — the swap is just the shared `pill_label` local, driven by `_topic_item`'s optional `available_pill:` local, default `false`). Non-available rows keep their real status pill. Always green — the mockup's blue "(You)" rows are an artifact. Display-only; never a substitute authorization gate (ADR 014). +## Course show (browse tables) + +- **browse table** — one of the three filterable lists on `courses/show`: Groups, Students (People tab), topics directory. Each is re-rendered wholesale on every filter change, so each has to answer for itself what an absence means. +- **base list** — a browse table's unfiltered, policy-scoped rows, held alongside its filtered form. The pair is what makes the two kinds of absence separable: the **base list** decides *which* one, never the unscoped list — a student who sees no approved topics has an empty **base list**, not a filtered one, because they filtered nothing out. +- **empty state** — a **browse table** with nothing in it, ever: no groups created, no students enrolled, nothing available to this viewer. Blames nothing and implies no filter is at fault. Distinct from **no-matches state**; the **profile empty state** below is the same idea on a different page. +- **no-matches state** — a **browse table** emptied by the active filters while its **base list** still holds rows. One generic pair of lines serves every filter ("No *nouns* match your current filters." + "Try adjusting your search or filters."), so there is no per-filter copy to fall out of date when a filter is added. +- **filters active** — at least one of a **browse table**'s filters narrows the list; a select's `all` does not count. Reported to the table as a local rather than re-derived, because it outlives the state: a search that *does* match is still **filters active** and must still auto-expand rows. + ## Participant profile - **participant profile** — the `courses#profile` page for one student or group, reached as `courses/profile/:participant_id/:participant_type` from the People/Groups rows (ADR 0016). Rendered in the **courses/show shell** (`bg-surface-tint`, inline `shared/sidebar` in the page's own `.flex`, `rounded-tl-panel` main — no `border-t border-l`) with content in a centered `max-w-5xl` reading column. diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index e831d08c..db9e97a1 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -3,6 +3,11 @@ # Handles CRUD for courses class CoursesController < ApplicationController + # Params that narrow a list. "all" is the selects' neutral choice and counts + # as inactive, as does an absent/blank param. + PARTICIPANT_FILTER_KEYS = %w[search_query lecturer_filter status_filter].freeze + TOPIC_FILTER_KEYS = %w[search_query topic_filter].freeze + before_action :set_course, only: %i[show add_students handle_add_students add_lecturers handle_add_lecturers settings handle_settings destroy export_csv profile update_coursecode update_email_domain grouping_preview] before_action :set_lecturer_enrolments, only: %i[settings handle_settings] @@ -25,7 +30,7 @@ def show # Topics Directory (topics_by_supervisor) data source — policy-scoped with # search/filter applied server-side; driving both the initial render and the # htmx re-render of _topics_by_supervisor_list. - @filtered_topic_list = filtered_topic_list + @filtered_topic_list = filtered_topic_list.to_a @topics_by_supervisor = topics_by_supervisor # set students projects @@ -105,17 +110,28 @@ def show @filtered_student_list = filtered_student_list @show_all = params[:show_all] == 'true' + # Counts the matches, taken before the truncation below: the table footer's + # "Showing X of Y" is about the current criteria, not the course total. @total_group_count = @filtered_group_list.count @total_student_count = @filtered_student_list.count - @total_count = @course.grouped? ? @total_group_count : @total_student_count - @total_count = @course.grouped? ? @filtered_group_list.count : @filtered_student_list.count unless @show_all @filtered_group_list = @filtered_group_list.first(Rails.application.config.participants_pagination_threshold) @filtered_student_list = @filtered_student_list.first(Rails.application.config.participants_pagination_threshold) end - @displayed_count = @course.grouped? ? @filtered_group_list.count : @filtered_student_list.count + # Empty vs no-matches, decided here rather than in the partials (ADR 0018). + # Each base is the unfiltered, policy-scoped list and is already loaded, so + # this costs no extra query. Computed after the truncation so the state and + # the list the partial receives can never disagree. + @group_list_state = list_state(@group_list, @filtered_group_list) + @student_list_state = list_state(@student_list, @filtered_student_list) + @topic_list_state = list_state(@topic_list, @filtered_topic_list) + + # The filter controls sit outside the htmx-swapped containers, so the + # partials are told a filter is active rather than re-deriving it from params. + @filters_active = filters_active?(PARTICIPANT_FILTER_KEYS) + @topic_filters_active = filters_active?(TOPIC_FILTER_KEYS) @capacity_result = SupervisorCapacityCalculator.new(@course).calculate @lecturer_capacity_info = @capacity_result.lecturer_capacities.index_by { |lc| lc.enrolment.user_id } @@ -130,7 +146,9 @@ def show projects_by_owner: @projects_by_owner, total_count: @total_group_count, displayed_count: @filtered_group_list.count, - show_all: @show_all + show_all: @show_all, + state: @group_list_state, + filters_active: @filters_active } elsif params[:section] == 'topics' render partial: 'topics_by_supervisor_list', @@ -138,7 +156,9 @@ def show course: @course, lecturers: @lecturers, topics_by_supervisor: @topics_by_supervisor, - current_user_enrolment: @current_user_enrolment + current_user_enrolment: @current_user_enrolment, + state: @topic_list_state, + filters_active: @topic_filters_active } else render partial: 'students_table', @@ -150,7 +170,9 @@ def show total_student_count: @student_list.count, total_count: @total_student_count, displayed_count: @filtered_student_list.count, - show_all: @show_all + show_all: @show_all, + state: @student_list_state, + filters_active: @filters_active } end nil @@ -869,6 +891,29 @@ def groups_by_status(status, group_list, course) # Participants Table Filters helpers + # A list renders one of three states, and only the first two are ever + # displayed — a non-empty list renders rows and never consults the state: + # + # :matched — the filter returned rows + # :no_matches — the base has rows, the filter returned none + # :empty — the base itself is empty; nothing has ever existed here + # + # The base is always the unfiltered, policy-scoped list, so a viewer whose + # policy scope hides everything is :empty rather than a false "no matches" + # (a student on a course with no approved topics has not filtered anything + # out). Both arguments are loaded by the time this runs (ADR 0018). + def list_state(base_list, filtered_list) + return :matched if filtered_list.any? + + base_list.any? ? :no_matches : :empty + end + + # True when any of the given filter params narrows the list. "all" is the + # selects' neutral choice, so it does not count. + def filters_active?(keys) + keys.any? { |key| params[key].present? && params[key] != 'all' } + end + def search_groups(group_list, query) downcased_query = query.downcase diff --git a/app/controllers/homescreen_controller.rb b/app/controllers/homescreen_controller.rb index 8e4fe321..57a6435c 100644 --- a/app/controllers/homescreen_controller.rb +++ b/app/controllers/homescreen_controller.rb @@ -1,5 +1,5 @@ -class HomescreenController < ApplicationController - def show - @courses = Current.user.courses.uniq - end -end +class HomescreenController < ApplicationController + def show + @courses = Current.user.courses_by_earliest_enrolment + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 0dac08a3..9cc9306b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -8,6 +8,14 @@ class User < ApplicationRecord has_many :enrolments, dependent: :destroy has_many :courses, through: :enrolments + # sort cards by earliest enrolment (coordinators can have multiple). + def courses_by_earliest_enrolment + Course.joins(:enrolments) + .where(enrolments: { user_id: id }) + .group('courses.id') + .order(Arel.sql('MIN(enrolments.created_at) DESC, courses.id DESC')) + end + has_many :project_group_members, dependent: :destroy has_many :project_groups, through: :project_group_members diff --git a/app/views/courses/_groups_tab.html.erb b/app/views/courses/_groups_tab.html.erb index e50c9e04..09adbdb5 100644 --- a/app/views/courses/_groups_tab.html.erb +++ b/app/views/courses/_groups_tab.html.erb @@ -7,7 +7,7 @@

Groups

- <%= pluralize(@total_group_count, "group") %> + <%= pluralize(@group_list.size, "group") %>
@@ -87,7 +87,9 @@ projects_by_owner: @projects_by_owner, total_count: @total_group_count, displayed_count: @filtered_group_list.count, - show_all: @show_all %> + show_all: @show_all, + state: @group_list_state, + filters_active: @filters_active %> <%= render "courses/add_students_modal", course: @course %> diff --git a/app/views/courses/_groups_table.html.erb b/app/views/courses/_groups_table.html.erb index f99e6837..b4741a19 100644 --- a/app/views/courses/_groups_table.html.erb +++ b/app/views/courses/_groups_table.html.erb @@ -1,4 +1,8 @@ -<%# --- locals: course, groups, projects_by_owner, total_count, displayed_count, show_all %> +<%# --- locals: course, groups, projects_by_owner, total_count, displayed_count, show_all, state, filters_active %> +<%# htmx re-render target on the Groups tab, rendered from BOTH the page path and + the courses#show htmx branch, so it only ever uses locals. `state` is the + controller's :matched / :no_matches / :empty verdict (ADR 0018); the copy + for each is chosen here, below. %>
<% if groups.present? %> - <% any_filter = params[:search_query].present? || - (params[:lecturer_filter].present? && params[:lecturer_filter] != 'all') || - (params[:status_filter].present? && params[:status_filter] != 'all') %> <% groups.each do |group| %> <% group_project = projects_by_owner[['ProjectGroup', group.id]] group_status = group_project&.current_status || 'not_submitted' @@ -34,14 +35,13 @@ project: group_project, status: group_status, supervisor: supervisor, - expanded: any_filter %> + expanded: filters_active %> <% end %> <% else %> - - - <%= params[:status_filter].present? && params[:status_filter] != 'all' ? "No groups found with #{params[:status_filter].titleize} status." : "No groups have been created yet." %> - - + <%= render "shared/table_empty_state", + colspan: 5, + heading: state == :no_matches ? "No groups match your current filters." : "No groups have been created yet.", + hint: state == :no_matches ? "Try adjusting your search or filters." : nil %> <% end %> diff --git a/app/views/courses/_people_tab.html.erb b/app/views/courses/_people_tab.html.erb index f25cec25..6cfeac47 100644 --- a/app/views/courses/_people_tab.html.erb +++ b/app/views/courses/_people_tab.html.erb @@ -15,4 +15,6 @@ total_student_count: @student_list.count, total_count: @total_student_count, displayed_count: @filtered_student_list.count, - show_all: @show_all %> + show_all: @show_all, + state: @student_list_state, + filters_active: @filters_active %> diff --git a/app/views/courses/_students_section.html.erb b/app/views/courses/_students_section.html.erb index e4656fa4..3d32e661 100644 --- a/app/views/courses/_students_section.html.erb +++ b/app/views/courses/_students_section.html.erb @@ -1,4 +1,4 @@ -<%# --- locals: course, students, student_group_map, student_enrolment_map, total_student_count, total_count, displayed_count, show_all %> +<%# --- locals: course, students, student_group_map, student_enrolment_map, total_student_count, total_count, displayed_count, show_all, state, filters_active %> <%# The students-select controller scopes this whole section: it drives the single-select radios, the Actions dropdown dispatch, and survives the htmx table swaps (delegated listeners + htmx:afterSwap reset). %> @@ -82,7 +82,9 @@ student_enrolment_map: student_enrolment_map, total_count: total_count, displayed_count: displayed_count, - show_all: show_all %> + show_all: show_all, + state: state, + filters_active: filters_active %> <%= render "courses/add_students_modal", course: course %> diff --git a/app/views/courses/_students_table.html.erb b/app/views/courses/_students_table.html.erb index 8475600a..fc116786 100644 --- a/app/views/courses/_students_table.html.erb +++ b/app/views/courses/_students_table.html.erb @@ -1,7 +1,9 @@ -<%# --- locals: course, students, student_group_map, student_enrolment_map, total_count, displayed_count, show_all %> +<%# --- locals: course, students, student_group_map, student_enrolment_map, total_count, displayed_count, show_all, state, filters_active %> <%# htmx re-render target on the People tab — rendered from BOTH the page path and the courses#show htmx branch, so it only ever uses locals. Never reach for - student_status/student_project_for (see plan §5). %> + student_status/student_project_for (see plan §5). `state` is the controller's + :matched / :no_matches / :empty verdict (ADR 0018); the copy for each is + chosen here, below. %>
@@ -24,9 +26,10 @@ enrolment: student_enrolment_map[student.id] %> <% end %> <% else %> - - - + <%= render "shared/table_empty_state", + colspan: 4, + heading: state == :no_matches ? "No students match your current filters." : "No students have been enrolled yet.", + hint: state == :no_matches ? "Try adjusting your search or filters." : nil %> <% end %>
No students have been enrolled yet.
diff --git a/app/views/courses/_topic_directory_tab.html.erb b/app/views/courses/_topic_directory_tab.html.erb index 2b41ddb2..d5db87fd 100644 --- a/app/views/courses/_topic_directory_tab.html.erb +++ b/app/views/courses/_topic_directory_tab.html.erb @@ -9,7 +9,9 @@ course: @course, lecturers: @lecturers, topics_by_supervisor: @topics_by_supervisor, - current_user_enrolment: @current_user_enrolment %> + current_user_enrolment: @current_user_enrolment, + state: @topic_list_state, + filters_active: @topic_filters_active %> <% else %> <%= render "shared/empty_state", heading: "Topics are disabled for this course", diff --git a/app/views/courses/_topics_by_supervisor_list.html.erb b/app/views/courses/_topics_by_supervisor_list.html.erb index 23008798..cad7d300 100644 --- a/app/views/courses/_topics_by_supervisor_list.html.erb +++ b/app/views/courses/_topics_by_supervisor_list.html.erb @@ -1,21 +1,26 @@ <%# Topics Directory body — the htmx-swappable container (mirror of _groups_table/_students_table: hx-target + hx-swap="outerHTML" replaces this - div). Locals: course, topics_by_supervisor, current_user_enrolment. - Supervisor groups: header (name + gray "(You)" for the viewer's own group + - topic count + chevron) over a list of _topic_item rows. Groups render - expanded by default (mockup). Under an active search/filter, non-matching - groups are dropped entirely rather than shown empty. When no supervisors - exist at all, the lone empty-state sentence renders. %> + div). Locals: course, topics_by_supervisor, current_user_enrolment, state, + filters_active. Supervisor groups: header (name + gray "(You)" for the + viewer's own group + topic count + chevron) over a list of _topic_item rows. + Groups render expanded by default (mockup). Under an active search/filter, + non-matching groups are dropped entirely rather than shown empty — so a + filter that matches nothing drops every group and the lone empty-state + sentence below takes over. `state` is the controller's :matched / + :no_matches / :empty verdict (ADR 0018); the copy for each is chosen here. %> -<% - any_filter = params[:search_query].present? || - (params[:topic_filter].present? && params[:topic_filter] != 'all') - visible = any_filter ? topics_by_supervisor.reject { |_lecturer, topics| topics.empty? } : topics_by_supervisor -%> +<% visible = filters_active ? topics_by_supervisor.reject { |_lecturer, topics| topics.empty? } : topics_by_supervisor %>
<% if visible.empty? %> -

No topics are currently available.

+
+

+ <%= state == :no_matches ? "No topics match your current filters." : "No topics are currently available." %> +

+ <% if state == :no_matches %> +

Try adjusting your search or filters.

+ <% end %> +
<% else %> <% visible.each do |lecturer, topics| %>
diff --git a/app/views/courses/_topics_section.html.erb b/app/views/courses/_topics_section.html.erb index 34ab9e83..73341c1b 100644 --- a/app/views/courses/_topics_section.html.erb +++ b/app/views/courses/_topics_section.html.erb @@ -1,5 +1,6 @@ <%# Topics Directory — the browse-first Topics panel of courses/show (ADR 0013). - Locals: course, lecturers, topics_by_supervisor, current_user_enrolment. + Locals: course, lecturers, topics_by_supervisor, current_user_enrolment, + state, filters_active. The section carries data-controller="expandable-rows" so the supervisor header toggles and the Collapse all / Expand all control keep working when htmx swaps the inner #topics-by-supervisor-container (mirror of the Groups @@ -67,5 +68,7 @@ <%= render "courses/topics_by_supervisor_list", course: course, topics_by_supervisor: topics_by_supervisor, - current_user_enrolment: current_user_enrolment %> + current_user_enrolment: current_user_enrolment, + state: state, + filters_active: filters_active %> diff --git a/app/views/shared/_list_item.html.erb b/app/views/shared/_list_item.html.erb index 1fe84262..089a52fb 100644 --- a/app/views/shared/_list_item.html.erb +++ b/app/views/shared/_list_item.html.erb @@ -2,7 +2,12 @@ time_meta (appended to meta line as "• timestamp"), status, pill_label (defaults to status humanized), linkable (default true), link_options, border_top (default true), and an optional - block rendered below the row line. The status pill sits on the right. %> + block rendered below the row line. The status pill sits on the right. + Style locals (all default-preserving — omit to keep current chrome): + hide_more_vert (drop the action button), title_weight (default 400), + density (:comfortable = roomier row: p-5, hover:bg-surface-hover, + w-10 h-10 icon, py-1 pill), meta_first_strong (first meta part + font-medium text-on-surface-variant). %> <% pill_bg, pill_fg = @@ -31,22 +36,33 @@ border_top = local_assigns.fetch(:border_top, true) rejected = status.to_s == "rejected" + hide_more_vert = local_assigns.fetch(:hide_more_vert, false) + title_weight = local_assigns.fetch(:title_weight, 400) + dense = local_assigns.fetch(:density, :default) == :comfortable + meta_first_strong = local_assigns.fetch(:meta_first_strong, false) + + row_padding = dense ? "p-5" : "py-3 px-4" + row_hover = dense ? "hover:bg-surface-hover" : "hover:bg-surface-variant" + row_classes = - "group flex flex-col w-full py-3 px-4 " \ - "hover:bg-surface-variant transition-colors bg-white" + "group flex flex-col w-full #{row_padding} " \ + "#{row_hover} transition-colors bg-white" row_classes += " border-t border-outline-variant" if border_top row_classes += " opacity-80" if rejected row_classes += linkable ? " cursor-pointer" : " opacity-75 cursor-not-allowed" title_color = rejected ? "text-on-surface-variant" : "text-on-surface-strong" - pill_span = "#{pill_bg} #{pill_fg} px-2.5 py-0.5 rounded text-[12px] font-medium tracking-wide whitespace-nowrap" + pill_padding = dense ? "py-1" : "py-0.5" + pill_span = "#{pill_bg} #{pill_fg} px-2.5 #{pill_padding} rounded text-[12px] font-medium tracking-wide whitespace-nowrap" + + icon_size = dense ? "w-10 h-10" : "w-9 h-9" %> <% content_block = capture do %>
-
+
<%= icon %>
@@ -55,11 +71,15 @@
-

<%= title %>

+

<%= title %>

<% meta_parts.each_with_index do |part, idx| %> <% unless idx.zero? %>•<% end %> - "><%= part %> + <% if idx.zero? && meta_first_strong %> + <%= part %> + <% else %> + "><%= part %> + <% end %> <% end %> <% if time_meta.present? %> <% if meta_parts.any? %>•<% end %> @@ -71,10 +91,12 @@
<%= pill_label %> - + <% unless hide_more_vert %> + + <% end %>
diff --git a/app/views/shared/_sidebar.html.erb b/app/views/shared/_sidebar.html.erb index fbfc7f26..6526e000 100644 --- a/app/views/shared/_sidebar.html.erb +++ b/app/views/shared/_sidebar.html.erb @@ -18,7 +18,7 @@ expand_less
- <% current_user.courses.distinct.each do |course| %> + <% current_user.courses_by_earliest_enrolment.each do |course| %> <%= render "shared/sidebar_nav_item", kind: :course, path: course_path(course), diff --git a/app/views/shared/_table_empty_state.html.erb b/app/views/shared/_table_empty_state.html.erb new file mode 100644 index 00000000..17c35717 --- /dev/null +++ b/app/views/shared/_table_empty_state.html.erb @@ -0,0 +1,16 @@ +<%# locals: (colspan:, heading:, hint: nil) %> +<%# The one row a filtered-to-nothing table shows, in place of its body rows. + Two lines so a filter miss can point at the way out without naming the + filter (the caller owns that wording). shared/_empty_state is the + illustrated card for a whole page or section and cannot go inside a + — this is the table-shaped counterpart, for the Groups and People + tables. Which of the two empty situations this is was decided by the + controller (`state:`); the copy is the caller's (ADR 0018). %> + + +

<%= heading %>

+ <% if hint.present? %> +

<%= hint %>

+ <% end %> + + diff --git a/app/views/styleguide/show.html.erb b/app/views/styleguide/show.html.erb index 1359a5e3..7c8830e2 100644 --- a/app/views/styleguide/show.html.erb +++ b/app/views/styleguide/show.html.erb @@ -260,6 +260,13 @@
The optional block renders below the row line — used for inline annotations.
<% end %>
+

Roomier picker rows — density: :comfortable, title_weight: 500, hide_more_vert: true, meta_first_strong: true (used by the copy-topic modal, step 1).

+
+ <%= render "shared/list_item", path: "#", icon: "topic", title: "Edge Computing for Rural IoT", meta_parts: ["Dr. Leong", "Software Engineering Project"], time_meta: "Updated 5 hours ago", status: "approved", + density: :comfortable, title_weight: 500, hide_more_vert: true, meta_first_strong: true %> + <%= render "shared/list_item", path: "#", icon: "topic", title: "Federated Learning in Healthcare", meta_parts: ["Siti Aminah", "Machine Learning Applications"], time_meta: "Updated yesterday", status: "approved", + density: :comfortable, title_weight: 500, hide_more_vert: true, meta_first_strong: true %> +
diff --git a/app/views/topics/_copy_topic_details.html.erb b/app/views/topics/_copy_topic_details.html.erb index 2bdb1a3d..4ea10d07 100644 --- a/app/views/topics/_copy_topic_details.html.erb +++ b/app/views/topics/_copy_topic_details.html.erb @@ -1,132 +1,131 @@ <%= turbo_frame_tag "overlay_content", class: "flex flex-col h-full" do %> - <%= form_with url: import_details_course_path(target), method: :post, class: "flex flex-col h-full max-h-[75vh]" do |f| %> + <%= form_with url: import_details_course_path(target), method: :post, class: "flex flex-col h-full max-h-[85dvh]" do |f| %> <%= f.hidden_field :source_topic_id, value: source.id, id: "overlay_source_topic_id" %> -
-

- Copying from "<%= source.current_title %>" - - to new topic in "<%= target.course_name %>" - -

+ +
+
+

+ Copy details from "<%= source.current_title %>" +

+

+ Mapping to new topic in "<%= target.course_name %>" +

+
-
-
    - <% current_instance = source.topic_instances.order(version: :desc).first %> - <% fields = current_instance.project_instance_fields.includes(:project_template_field) %> + <% current_instance = source.topic_instances.order(version: :desc).first %> + <% fields = current_instance.project_instance_fields.includes(:project_template_field) %> + + +
    +
    <% @template_fields.each do |field| %> -
  • - <% - initial_field = fields.find { |f| f.project_template_field.label == field.label } - show_preview = false + <% + initial_field = fields.find { |f| f.project_template_field.label == field.label } + show_preview = false - if initial_field.present? - source_value = initial_field.value.to_s.strip - if ["dropdown", "radio"].include?(field.field_type.downcase) - allowed_options = (field.options || []).map { |opt| opt.to_s.strip } - show_preview = source_value.blank? || allowed_options.include?(source_value) - else - show_preview = true - end + if initial_field.present? + source_value = initial_field.value.to_s.strip + if ["dropdown", "radio"].include?(field.field_type.downcase) + allowed_options = (field.options || []).map { |opt| opt.to_s.strip } + show_preview = source_value.blank? || allowed_options.include?(source_value) + else + show_preview = true end - %> + end + %> -
    -
    - - <%= field.label %> - - - - <%= field.field_type_label %> - +
    +
    +
    + <%= field.label %> + <%= field.field_type_label %>
    +
    -
    - <% - valid_options = fields.select do |instance_field| - opt_value = instance_field.value.to_s.strip - if ["dropdown", "radio"].include?(field.field_type.downcase) - allowed_options = (field.options || []).map { |opt| opt.to_s.strip } - opt_value.blank? || allowed_options.include?(opt_value) - else - true - end +
    + <% + valid_options = fields.select do |instance_field| + opt_value = instance_field.value.to_s.strip + if ["dropdown", "radio"].include?(field.field_type.downcase) + allowed_options = (field.options || []).map { |opt| opt.to_s.strip } + opt_value.blank? || allowed_options.include?(opt_value) + else + true end - %> - - Copy from: - - -
    -
    - + - <% if is_valid_choice %> - - <% end %> - <% end %> - + <% fields.each do |instance_field| %> + <% + opt_value = instance_field.value.to_s.strip + is_valid_choice = true -
    - - - -
    -
    + if ["dropdown", "radio"].include?(field.field_type.downcase) + allowed_options = (field.options || []).map { |opt| opt.to_s.strip } + is_valid_choice = opt_value.blank? || allowed_options.include?(opt_value) + end + %> -
    - Preview Content -

    - <%= initial_field&.value if show_preview %> -

    -
    + <% if is_valid_choice %> + + <% end %> + <% end %> + +
    + + +
    + + +
    +

    Preview

    +

    + <%= initial_field&.value if show_preview %> +

    +
    -
    • +
    <% end %> -
+ +
-
+ +
diff --git a/app/views/topics/_copy_topic_list_item.html.erb b/app/views/topics/_copy_topic_list_item.html.erb new file mode 100644 index 00000000..d79327e0 --- /dev/null +++ b/app/views/topics/_copy_topic_list_item.html.erb @@ -0,0 +1,33 @@ +<%# Copy-topic picker row: a step-1 copy-topic modal list_item rendering (mirrors + the domain-wrapper pattern of courses/_topic_item). Locals: topic (required), + border_top (passed through to shared/_list_item, default true — the caller + drops it on the first row so it sits flush under the sticky header). Rows + are always linkable approved topics loading the source-picker (step 2) via + the copy-topic controller. Uses shared/_list_item's picker style locals: + density :comfortable, title_weight 500, hide_more_vert, meta_first_strong. + Course name lives in the meta line (owner • course • Updated X ago), not a + below-row chip — that's the step-1 mockup's softer hierarchy. %> + +<% topic_instance = topic.topic_instances.order(:version).last %> + +<% if topic_instance %> + <%= render "shared/list_item", + path: new_course_topic_path(@course, source_topic_id: topic.id), + icon: "topic", + title: topic_instance.title.presence || "Untitled Topic", + meta_parts: [topic.owner_name, topic.course.course_name], + time_meta: "Updated #{time_ago_in_words(topic.updated_at)} ago", + status: "approved", + density: :comfortable, + title_weight: 500, + hide_more_vert: true, + meta_first_strong: true, + border_top: local_assigns.fetch(:border_top, true), + link_options: { + data: { + copy_topic_target: "card", + action: "click->copy-topic#selectTopic", + copy_topic_course_id_param: topic.id + } + } %> +<% end %> diff --git a/app/views/topics/_copy_topic_overlay.html.erb b/app/views/topics/_copy_topic_overlay.html.erb index e61277e6..22d2e8ce 100644 --- a/app/views/topics/_copy_topic_overlay.html.erb +++ b/app/views/topics/_copy_topic_overlay.html.erb @@ -16,83 +16,68 @@ <%= turbo_frame_tag "overlay_content", - class: "flex flex-col h-full p-5 md:p-8 overflow-y-auto", + class: "flex flex-col h-full", data: { copy_topic_target: "container" } do %> -
-

- Select a Topic to Copy -

+
-
- Show all topics from coordinated courses + +
+

+ Select a Topic to Copy +

- <%= form_with url: new_course_topic_path(@course), method: :get, data: { turbo_frame: "overlay_content" } do |f| %> - - <% end %> -
-
- -
- <% if @approved_topics.present? %> -
- <% @approved_topics.each do |topic| %> - <% topic_instance = topic.topic_instances.order(:version).last %> +
+ Show all topics from coordinated courses - <%= render "shared/list_item", - path: new_course_topic_path(@course, source_topic_id: topic.id), - icon: "topic", - title: topic_instance.title.presence || "Untitled Topic", - meta_parts: [topic.owner_name], - time_meta: "Updated #{time_ago_in_words(topic.updated_at)} ago", - status: "approved", - link_options: { - data: { - copy_topic_target: "card", - action: "click->copy-topic#selectTopic", - copy_topic_course_id_param: topic.id - } - } do %> -
- Course - - <%= topic.course.course_name %> - -
- <% end %> + <%= form_with url: new_course_topic_path(@course), method: :get, data: { turbo_frame: "overlay_content" } do |f| %> + <% end %>
- <% else %> -
- <%= image_tag "info.svg", class: "w-6 h-6" %> -
- You don't have any approved topics yet. -
-
- <% end %> -
+
+ + +
+ <% if @approved_topics.present? %> +
+ <% @approved_topics.each_with_index do |topic, idx| %> + <%= render "topics/copy_topic_list_item", topic: topic, border_top: !idx.zero? %> + <% end %> +
+ <% else %> +
+ <%= image_tag "info.svg", class: "w-6 h-6" %> +
+ You don't have any approved topics yet. +
+
+ <% end %> +
+ + +
+ +
-
-
<% end %>
diff --git a/app/views/topics/_template_fields.html.erb b/app/views/topics/_template_fields.html.erb index e641c683..581a5f78 100644 --- a/app/views/topics/_template_fields.html.erb +++ b/app/views/topics/_template_fields.html.erb @@ -36,7 +36,10 @@ <%= text_area_tag "fields[#{field.id}]", existing_value, class: "#{input_classes} resize-none overflow-hidden leading-relaxed", - data: { controller: "markdown-editor" }, + data: { + controller: "markdown-editor", + action: "text-editor:update->markdown-editor#setValue", + }, rows: 2, placeholder: field.hint, required: field.required %> diff --git a/docs/adr/0017-copy-topic-list-item-reuses-shared-locals.md b/docs/adr/0017-copy-topic-list-item-reuses-shared-locals.md new file mode 100644 index 00000000..0bc510b2 --- /dev/null +++ b/docs/adr/0017-copy-topic-list-item-reuses-shared-locals.md @@ -0,0 +1,63 @@ +# ADR 017 — Copy-topic step-1 rows reuse shared/list_item via defaults-preserving locals + +Date: 2026-09-24 +Status: Accepted + +## Context + +The "Select a Topic to Copy" list (step 1 of the `_copy_topic_overlay` modal) is +being redesigned to the `copy_topic_overlay_mockup`: a sticky header/footer with +a center-scrolling approved-topic list. The mockup's rows differ from today's +`shared/_list_item` chrome — roomier `p-5` padding, `hover:bg-surface-hover`, +`w-10 h-10` icon, `font-medium` title, no `more_vert` action button, and the +owner emphasized in the metadata line. The course name moves into the meta line +(`owner • course • Updated X ago`), replacing the old below-row "Course" chip +block. + +`shared/_list_item` already hard-codes that chrome for every call site +(`_overview_tab`, `_topic_item`, `projects/_list_item`, the styleguide). The +tempting shortcuts are both wrong: (a) copy the row markup into the overlay +partial as a bespoke third row language that diverges from the shared one, or +(b) change the shared component's defaults app-wide to match the mockup, which +would silently re-skin the Overview, topic directory, profiles, and styleguide. + +The codebase already has the right mechanism for "reuse a list row, vary its +chrome": domain wrapper partials (`courses/_topic_item`, `projects/_list_item`) +that build a `render_options` hash for `shared/_list_item`, gated by locals. + +## Decision + +Extend `shared/_list_item` with four default-preserving style locals, and render +the copy-topic rows through a new thin domain wrapper in +`topics/_copy_topic_list_item.html.erb` (mirroring `courses/_topic_item`), calling +`shared/_list_item` — never re-implementing row markup. + +The new locals (all omit-able; omission = today's byte-identical output): + +- `hide_more_vert` (bool, default `false`) — drop the hover-only action button. +- `title_weight` (int, default `400`) — title font weight; picker passes `500`. +- `density: :comfortable` (default `:default`) — one knob for the roomier row: + `p-5`, `hover:bg-surface-hover`, `w-10 h-10` icon, and pill `py-1`, all + together. +- `meta_first_strong` (bool, default `false`) — first meta part (the owner) + renders `font-medium text-on-surface-variant`. + +The overlay partial loops `@approved_topics` manually (not via `shared/_list`, +whose container hard-codes a `border-b` that would double the sticky footer's +border-t), passing `border_top: false` on the first row so it sits flush under +the header. Course name is passed as a second `meta_parts` entry. + +The dialog stays `max-w-4xl` (one shared ``, content-swapped by the turbo +frame; the step-1 mockup's `max-w-3xl` is stale). + +## Consequences + +- One row language: the picker, Overview, topic directory, profiles, and + styleguide all still render through `shared/_list_item`. +- Defaults are preserved; every existing call site renders byte-identically + (verified by the green suite). +- `shared/_list_item`'s local surface grows by four; each is narrow and + explicitly default-preserving, not a free-form class escape hatch. +- The overall copy-topic step 1 chrome (sticky header/footer, full-bleed scroll + region) lives in `_copy_topic_overlay.html.erb`; the rows live in the shared + component. Any future "roomier list" reuses `density: :comfortable` for free. \ No newline at end of file diff --git a/docs/adr/0018-empty-state-decided-in-the-controller.md b/docs/adr/0018-empty-state-decided-in-the-controller.md new file mode 100644 index 00000000..3f4f990a --- /dev/null +++ b/docs/adr/0018-empty-state-decided-in-the-controller.md @@ -0,0 +1,108 @@ +# ADR 0018 — A browse table's empty state is decided in the controller, from its base list + +Date: 2026-09-26 +Status: Accepted + +## Context + +The three filterable lists on `courses/show` — Groups, Students (People tab), +topics directory — are htmx targets (ADR 015) whose containers are swapped +wholesale, so each one is re-rendered by the `courses#show` htmx branch from +the same locals the page path passes. All three also have a filter: a search +box plus, for Groups and Students, status and supervisor selects. + +Each partial carried its own empty-state markup, and each one branched on the +filters *by reading `params` from the view*. `_groups_table.html.erb` handled +exactly one of its three filters, so a search that matched nothing, or a +supervisor filter that matched nothing, fell through to its default branch and +told the coordinator **"No groups have been created yet."** — a false claim +about their course. `_students_table.html.erb` never branched at all and always +said "No students have been enrolled yet." The topics directory's single +sentence, "No topics are currently available.", was at least neutral, but +equally indistinguishable from a filter miss. + +Two things were already true and made the split possible: + +- Each action holds the **unfiltered, policy-scoped** list and the filtered one + at the same instant — `@group_list` (`courses_controller.rb:47`) beside + `@filtered_group_list`, `@student_list` beside `@filtered_student_list`, + `@topic_list` beside `@filtered_topic_list`. Both are already loaded, so + comparing them costs no query. +- ADR 015 established the locals-not-ivars seam in spirit and the partials' + own header comments state it outright ("rendered from BOTH the page path and + the courses#show htmx branch, so it only ever uses locals"). Reading `params` + in a partial that re-renders on every keystroke contradicted the convention + the file declared two lines above the code that broke it. + +The interesting part is not the mechanism — it is that a filter miss and a +genuinely-empty list are different *claims about the world*, and the old code +had no way to tell them apart. + +## Decision + +1. `CoursesController#list_state(base_list, filtered_list)` returns one of three + symbols, and the partials receive it as a `state:` local: + + - `:matched` — the filter returned rows (the state is never rendered) + - `:no_matches` — the base has rows, the filter returned none + - `:empty` — the base itself is empty + +2. The base is always the **unfiltered, policy-scoped** list, never the + unscoped one. A student on a course with no approved topics has an empty + base — they filtered nothing out, so "no matches" would be its own lie. + +3. The base and the filtered list are both compared **after** the 25-row + truncation, so the state and the rows the partial receives can never + disagree. + +4. The controller hands over a **symbol**; the view owns the copy. `state` says + which situation this is, and the partial maps it to words. User-facing + strings stay in the view, matching how `shared/_empty_state` already takes + `heading:`/`description:` from its callers. + +5. One generic no-matches message serves every filter — "No *nouns* match your + current filters." plus a muted "Try adjusting your search or filters." With + three filters a per-filter message set is seven branches and seven tests, + each one a chance to be wrong the moment a filter is added. Specificity is + available in the control the user is looking at, not in the message. + +6. `filters_active:` is a second local, reported rather than re-derived. It + cannot be folded into `state`: under a search that *does* match, the state + is `:matched` but rows must still auto-expand, and the topics directory + must still drop its empty supervisor groups. It is computed from + `PARTICIPANT_FILTER_KEYS` / `TOPIC_FILTER_KEYS` (`"all"` being the selects' + neutral choice, so not active). + +7. The two table-shaped empties share `shared/_table_empty_state` + (`colspan:`, `heading:`, `hint:`), so Groups and Students cannot drift a + third time. The topics directory keeps its own `

` — a `` cannot be a + `

` — but takes the same two-state logic. `shared/_empty_state` is + untouched: its root is a `

` with a 180px illustration, unusable inside a + ``, and it has five other call sites. + +## Consequences + +- A filter miss can no longer assert that nothing exists. The regression guard + is the *negative* assertion in every no-matches test + (`assert_no_match 'No groups have been created yet.'`), which is the check + that would have caught the original defect. +- The three swapped partials no longer read `params` at all. The filter + *controls* (`_groups_tab`, `_students_section`, `_topics_section`) still do, + to echo their own current `value`/`selected` — a control reflecting state is + not a decision, and that is where the convention stops. +- The controller grows three ivars and two small private methods, and threads + two locals through the ivar→locals seam. Every future filterable list is + expected to pay that same small toll. +- Adding a filter no longer means writing copy. It means adding a key to + `PARTICIPANT_FILTER_KEYS`/`TOPIC_FILTER_KEYS` so `filters_active?` sees it. +- "Show all N" and the "Showing X of Y" footer are unchanged. `@total_group_count` + is the count of *matches* taken before truncation, so "Showing 25 of 40" under + an active search is correct, not a bug. `@total_count` (assigned twice) and + `@displayed_count` were read by no view and are deleted. + +## References + +- ADR 015 — the htmx decision this extends; it fixes the mechanism but is silent + on who decides what the swapped partial shows. +- ADR 013 — the topics directory's browse-first shape, which is why a student's + policy-scoped list is the meaningful base there. diff --git a/test/controllers/courses_controller_test.rb b/test/controllers/courses_controller_test.rb index 94296377..8e4ad93d 100644 --- a/test/controllers/courses_controller_test.rb +++ b/test/controllers/courses_controller_test.rb @@ -606,6 +606,133 @@ class CoursesControllerTest < ActionDispatch::IntegrationTest assert_no_match 'No project has been submitted yet.', response.body end + # --- Empty vs no-matches (ADR 0018) ------------------------------------- + # + # Every no-matches test asserts the *absence* of the genuinely-empty copy as + # well. That negative assertion is the regression guard: before the split, a + # search or supervisor filter that matched nothing fell through to the + # "nothing has ever existed here" branch and made a false claim about the + # course. + + test 'groups table reports an empty course when no groups exist' do + course = create(:course, :grouped) + create(:enrolment, :coordinator, user: @coordinator_user, course: course) + + sign_in @coordinator_user + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, params: { section: 'groups' } + assert_response :success + assert_includes response.body, 'No groups have been created yet.' + assert_no_match 'No groups match your current filters.', response.body + end + + test 'groups table reports no matches when a search empties the list' do + course = create(:course, :grouped) + create(:enrolment, :coordinator, user: @coordinator_user, course: course) + create(:project_group, course: course, confirmed: true, group_name: 'Alpha Group') + + sign_in @coordinator_user + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, + params: { section: 'groups', search_query: 'zzzznomatch' } + assert_response :success + assert_includes response.body, 'No groups match your current filters.' + assert_includes response.body, 'Try adjusting your search or filters.' + assert_no_match 'No groups have been created yet.', response.body + end + + test 'groups table reports no matches when a status filter empties the list' do + course = create(:course, :grouped) + create(:enrolment, :coordinator, user: @coordinator_user, course: course) + create(:project_group, course: course, confirmed: true, group_name: 'Alpha Group') + + sign_in @coordinator_user + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, + params: { section: 'groups', status_filter: 'approved' } + assert_response :success + assert_includes response.body, 'No groups match your current filters.' + assert_no_match 'No groups have been created yet.', response.body + # the pre-split per-filter copy is gone; one generic message serves every filter + assert_no_match 'No groups found with', response.body + end + + test 'groups table reports no matches when a supervisor filter empties the list' do + course = create(:course, :grouped) + create(:enrolment, :coordinator, user: @coordinator_user, course: course) + idle_lecturer = create(:user, :staff) + create(:enrolment, :lecturer, user: idle_lecturer, course: course) + create(:project_group, course: course, confirmed: true, group_name: 'Alpha Group') + + sign_in @coordinator_user + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, + params: { section: 'groups', lecturer_filter: idle_lecturer.id.to_s } + assert_response :success + assert_includes response.body, 'No groups match your current filters.' + assert_no_match 'No groups have been created yet.', response.body + end + + test 'students table reports an empty course when no students are enrolled' do + course = create(:course) + create(:enrolment, :coordinator, user: @coordinator_user, course: course) + + sign_in @coordinator_user + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, params: { section: 'students' } + assert_response :success + assert_includes response.body, 'No students have been enrolled yet.' + assert_no_match 'No students match your current filters.', response.body + end + + test 'students table reports no matches when a search empties the list' do + sign_in @coordinator_user + get course_path(@course), headers: { 'HTTP_HX_REQUEST' => 'true' }, + params: { section: 'students', search_query: 'zzzznomatch' } + assert_response :success + assert_includes response.body, 'No students match your current filters.' + assert_includes response.body, 'Try adjusting your search or filters.' + assert_no_match 'No students have been enrolled yet.', response.body + end + + test 'topics directory reports an empty course when it has no lecturers' do + course = create(:course) + create(:enrolment, :coordinator, user: @coordinator_user, course: course) + + sign_in @coordinator_user + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, params: { section: 'topics' } + assert_response :success + assert_includes response.body, 'No topics are currently available.' + assert_no_match 'No topics match your current filters.', response.body + end + + test 'topics directory reports no matches when a search empties every group' do + course, alice, = build_topic_directory_course + create_topic_on(course, alice, 'Machine Learning Basics', :approved) + + sign_in @coordinator_user + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, + params: { section: 'topics', search_query: 'zzzznomatch' } + assert_response :success + assert_includes response.body, 'No topics match your current filters.' + assert_includes response.body, 'Try adjusting your search or filters.' + assert_no_match 'No topics are currently available.', response.body + end + + test 'topics a student scope hides is an empty state, never a filter miss' do + course, alice, = build_topic_directory_course + create_topic_on(course, alice, 'Unapproved Topic', :pending) + + student = create(:user) + create(:enrolment, user: student, course: course) + sign_in student + + # The draft is outside a student's policy scope, so the *unfiltered* list is + # empty. With a search active the groups are dropped and the empty branch + # fires — it must read as "nothing to show you", not as a filter miss, since + # the base is the policy-scoped list (ADR 0018). + get course_path(course), headers: { 'HTTP_HX_REQUEST' => 'true' }, + params: { section: 'topics', search_query: 'anything' } + assert_response :success + assert_includes response.body, 'No topics are currently available.' + assert_no_match 'No topics match your current filters.', response.body + end + private def build_topic_directory_course diff --git a/test/integration/homescreen_cards_render_test.rb b/test/integration/homescreen_cards_render_test.rb index 3f8afec0..ba897b86 100644 --- a/test/integration/homescreen_cards_render_test.rb +++ b/test/integration/homescreen_cards_render_test.rb @@ -7,6 +7,27 @@ class HomescreenCardsRenderTest < ActionDispatch::IntegrationTest @course.enrolments.create!(user: @user, role: :student) end + test 'orders courses by earliest enrolment on the homescreen and sidebar' do + @course.enrolments.find_by!(user: @user, role: :student).update!(created_at: 4.days.ago) + + earlier_course = create(:course, course_name: 'Earlier Course') + create(:enrolment, user: @user, course: earlier_course, role: :student, created_at: 2.days.ago) + create(:enrolment, user: @user, course: earlier_course, role: :coordinator, created_at: 1.hour.ago) + + later_course = create(:course, course_name: 'Later Course') + create(:enrolment, user: @user, course: later_course, role: :student, created_at: 1.day.ago) + + post session_path, params: { email_address: @user.email_address, password: 'password' } + get root_path + + expected_paths = [course_path(later_course), course_path(earlier_course), course_path(@course)] + card_paths = css_select("main a[href^='/courses/']").map { |link| link['href'] } + sidebar_paths = css_select("#app-sidebar a[href^='/courses/']").map { |link| link['href'] } + + assert_equal expected_paths, card_paths + assert_equal expected_paths, sidebar_paths + end + test 'homescreen renders themed course cards via image_tag' do post session_path, params: { email_address: @user.email_address, password: 'password' } assert_redirected_to root_path diff --git a/test/system/topics/copy_topic_dialog_test.rb b/test/system/topics/copy_topic_dialog_test.rb index 14002fc7..38a1f7ad 100644 --- a/test/system/topics/copy_topic_dialog_test.rb +++ b/test/system/topics/copy_topic_dialog_test.rb @@ -11,7 +11,9 @@ class TopicCopyTopicDialogTest < BrowserSystemTestCase create(:enrolment, :lecturer, user: @lecturer, course: @course) # The course factory already creates a shorttext template field (Project Title). - # Add a second (dropdown) field for richer assertions. + # Add a dropdown (Supervisor) and a textarea (Description) field so the copy + # path is exercised for every input type — the textarea case regressed once + # when its text-editor:update bridge was dropped in the redesign. @template = @course.project_template @template_field = @template.project_template_fields.create!( label: 'Supervisor', @@ -21,6 +23,13 @@ class TopicCopyTopicDialogTest < BrowserSystemTestCase options: %w[Alice Bob], is_project_title: false ) + @description_field = @template.project_template_fields.create!( + label: 'Description', + field_type: 'textarea', + applicable_to: 'both', + required: false, + is_project_title: false + ) # A topic owned by the lecturer with an approved instance + filled fields @source_topic = create(:topic, course: @course, owner: @lecturer) @@ -40,34 +49,87 @@ class TopicCopyTopicDialogTest < BrowserSystemTestCase project_template_field: @template_field, value: 'Alice' ) + @source_instance.project_instance_fields.create!( + project_template_field: @description_field, + value: 'A detailed markdown description **with emphasis**' + ) end test 'opens dialog, shows source topic, loads step-2, and closes after copy' do login_as(@lecturer) visit new_course_topic_path(@course) + wait_for_turbo - # Step 1: click "Reuse details from another topic" → dialog opens - click_button 'Reuse details from another topic', wait: 3 - + # Step 1: click "Reuse details from another topic" → dialog opens. + # A physical Capybara click is intermittently swallowed by headless Chrome + # (the suite's known dropped-first-click race) even with wait_for_turbo, so + # fall back to a programmatic click if the dialog didn't stick. + assert_selector '#topic-form', wait: 3 + trigger = find("button[data-action='click->copy-topic#open']", wait: 3) + unless page.has_selector?('dialog[open]', wait: 2) + trigger.click + unless page.has_selector?('dialog[open]', wait: 2) + page.execute_script(<<~JS) + document.querySelector("button[data-action='click->copy-topic#open']").click() + JS + end + end assert_selector 'dialog[open]', wait: 3 assert_text 'Source Topic', wait: 3 - # Click the source topic card → turbo frame loads step-2 + # Click the source topic card → turbo frame loads step-2 (same physical- + # click race; fall back to a programmatic click if the frame didn't move). within 'dialog' do - find('a', text: 'Source Topic').click + card = find('a', text: 'Source Topic') + unless page.has_text?('Copy details from', wait: 2) + card.click + unless page.has_text?('Copy details from', wait: 2) + page.execute_script(<<~JS) + [...document.querySelectorAll('dialog a')].find((a) => a.textContent.includes('Source Topic')).click() + JS + end + end end - assert_text 'Copying from', wait: 3 + assert_text 'Copy details from', wait: 3 within 'dialog' do assert_text 'Supervisor', wait: 3 end # Click "Copy Details" → values transfer to the main form, dialog closes + # (same physical-click race; fall back to a programmatic click if the + # dialog doesn't close). + # Click "Copy Details" → values transfer to the main form, dialog closes + # (same physical-click race; fall back to a programmatic click if the + # dialog doesn't close). The open-state check must run at page scope, not + # inside `within 'dialog'` (which would only find a nested dialog). within 'dialog' do - click_button 'Copy Details' + find('button', text: 'Copy Details').click + end + unless page.has_no_selector?('dialog[open]', wait: 2) + page.execute_script(<<~JS) + [...document.querySelectorAll('dialog button')].find((b) => b.textContent.includes('Copy Details')).click() + JS end assert_no_selector 'dialog[open]', wait: 3 + + # The copied description must reach the EasyMDE editor on the main topic + # form (regression: the text-editor:update bridge was dropped in the + # redesign, so textarea copies never populated the visible editor — the + # raw textarea value alone synced back empty via forceSync). + editor_text = page.evaluate_script(<<~JS) + (() => { + const ta = document.querySelector("textarea[name='fields[#{@description_field.id}]']"); + if (!ta) return null; + const wrapper = ta.closest('.space-y-1'); + const cm = wrapper && wrapper.querySelector('.CodeMirror'); + return cm ? cm.textContent : null; + })() + JS + assert_includes editor_text.to_s, + 'A detailed markdown description', + 'visible EasyMDE editor should show the copied description' end test 'topics/edit renders the modern takeover layout and template fields' do