Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ This repository contains the **Better Together Community Engine** (an isolated R
- **Use allow-lists for dynamic class resolution**: Follow the `joatu_source_class` pattern with concern-based allow-lists
- **Validate user inputs**: Always sanitize and validate parameters, especially for file uploads and dynamic queries
- **Strong parameters**: Use Rails strong parameters in all controllers
- **Model-level permitted attributes**: Prefer defining a class method `self.permitted_attributes` on models that returns the permitted attribute array (including nested attributes). Controllers and shared resource code should call `Model.permitted_attributes` rather than hard-coding permit lists. Compose nested permitted attributes by referencing other models' `permitted_attributes` (for example: `Conversation.permitted_attributes` may include `{ messages_attributes: Message.permitted_attributes }`).
- **Authorization everywhere**: Implement Pundit policy checks on all actions
- **SQL injection prevention**: Use parameterized queries, avoid string interpolation in SQL
- **XSS prevention**: Use Rails auto-escaping, sanitize HTML inputs with allowlists
Expand Down Expand Up @@ -205,6 +206,35 @@ This repository contains the **Better Together Community Engine** (an isolated R
- **Rails-Controller-Testing**: Add `gem 'rails-controller-testing'` to Gemfile for `assigns` method in controller tests.
- Toggle requires_invitation and provide invitation_code when needed for registration tests.

### Automatic test configuration & auth helper patterns

This repository provides an automatic test-configuration layer (see `spec/support/automatic_test_configuration.rb`) that sets up the host `Platform` and, where appropriate, performs authentication for request, controller, and feature specs so most specs do NOT need to call `configure_host_platform` manually.

- Automatic setup applies to specs with `type: :request`, `type: :controller`, and `type: :feature` by default.
- Use these example metadata tags to control authentication explicitly:
- `:as_platform_manager` or `:platform_manager` — login as the platform manager (elevated privileges)
- `:as_user`, `:authenticated`, or `:user` — login as a regular user
- `:no_auth` or `:unauthenticated` — ensure no authentication is performed for the example
- `:skip_host_setup` — skip host platform creation/configuration for this example

How it works:
- The test helper inspects example metadata and description text (describe/context). If the description contains keywords such as "platform manager", "admin", "authenticated", or "signed in", it will automatically set appropriate tags and perform the corresponding authentication.
- The helper creates a host `Platform` if one does not exist and marks the default setup wizard as completed.
- For request specs it uses HTTP login helpers (`login(email, password)`); for controller specs it uses Devise test helpers (`sign_in`); for feature specs it uses Capybara UI login flows.

Recommended usage:
- Prefer using metadata tags (`:as_platform_manager`, `:as_user`, `:skip_host_setup`) in the `describe` or `context` header when a test needs a specific authentication state. Example:

```ruby
RSpec.describe 'Creating a conversation', type: :request, :as_user do
# host platform and user login are automatically configured
end
```

- Avoid calling `configure_host_platform` manually in most specs; reserve manual calls for special cases (use `:skip_host_setup` to opt out of automatic config).

Note: The helper set lives under `spec/support/automatic_test_configuration.rb` and provides helpers like `configure_host_platform`, `find_or_create_test_user`, and `capybara_login_as_platform_manager` to use directly if needed by unusual tests.

### Testing Architecture Standards
- **Project Standard**: Use request specs (`type: :request`) for all controller testing to maintain consistency
- **Request Specs Advantages**: Handle Rails engine routing automatically through full HTTP stack
Expand Down
30 changes: 30 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ Instructions for GitHub Copilot and other automated contributors working in this
- Use allow-lists for dynamic class resolution (see `joatu_source_class` pattern)
- Sanitize and validate all user inputs
- Use strong parameters in controllers
- Define model-level permitted attributes: prefer a class method `self.permitted_attributes` on models that returns the permitted attribute list (including nested attribute structures). Controllers should call `Model.permitted_attributes` to build permit lists instead of hard-coding them. When composing nested attributes, reference other models' `permitted_attributes` (for example: `Conversation.permitted_attributes` may include `{ messages_attributes: Message.permitted_attributes }`).
- Implement proper authorization checks (Pundit policies)
- **For reflection-based features**: Create concerns with `included_in_models` class methods for safe dynamic class resolution
- **Post-generation security check**: Run `bin/dc-run bundle exec brakeman --quiet --no-pager -c UnsafeReflection,SQL,CrossSiteScripting` after major code changes
Expand Down Expand Up @@ -226,6 +227,35 @@ For every implementation plan, create acceptance criteria covering relevant stak
- **Required for**: Controller specs, request specs, feature specs, and any integration tests that involve routing or authentication.
- **Locale Parameters**: Engine controller tests require locale parameters (e.g., `params: { locale: I18n.default_locale }`) due to routing constraints.

### Automatic test configuration & auth helper patterns

This repository provides an automatic test-configuration layer (see `spec/support/automatic_test_configuration.rb`) that sets up the host `Platform` and, where appropriate, performs authentication for request, controller, and feature specs so most specs do NOT need to call `configure_host_platform` manually.

- Automatic setup applies to specs with `type: :request`, `type: :controller`, and `type: :feature` by default.
- Use these example metadata tags to control authentication explicitly:
- `:as_platform_manager` or `:platform_manager` — login as the platform manager (elevated privileges)
- `:as_user`, `:authenticated`, or `:user` — login as a regular user
- `:no_auth` or `:unauthenticated` — ensure no authentication is performed for the example
- `:skip_host_setup` — skip host platform creation/configuration for this example

How it works:
- The test helper inspects example metadata and description text (describe/context). If the description contains keywords such as "platform manager", "admin", "authenticated", or "signed in", it will automatically set appropriate tags and perform the corresponding authentication.
- The helper creates a host `Platform` if one does not exist and marks the default setup wizard as completed.
- For request specs it uses HTTP login helpers (`login(email, password)`); for controller specs it uses Devise test helpers (`sign_in`); for feature specs it uses Capybara UI login flows.

Recommended usage:
- Prefer using metadata tags (`:as_platform_manager`, `:as_user`, `:skip_host_setup`) in the `describe` or `context` header when a test needs a specific authentication state. Example:

```ruby
RSpec.describe 'Creating a conversation', type: :request, :as_user do
# host platform and user login are automatically configured
end
```

- Avoid calling `configure_host_platform` manually in most specs; reserve manual calls for special cases (use `:skip_host_setup` to opt out of automatic config).

Note: The helper set lives under `spec/support/automatic_test_configuration.rb` and provides helpers like `configure_host_platform`, `find_or_create_test_user`, and `capybara_login_as_platform_manager` to use directly if needed by unusual tests.

## Test Coverage Standards
- **Models**: Test validations, associations, scopes, instance methods, class methods, and callbacks.
- **Controllers**: Test all actions, authorization policies, parameter handling, and response formats.
Expand Down
14 changes: 14 additions & 0 deletions app/assets/stylesheets/better_together/_resource_toolbar.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Styles specific to the shared resource toolbar

.resource-toolbar {
display: flex;
align-items: center;
}

.resource-toolbar-extra {
display: flex;
align-items: center;
margin-left: auto; // Fallback in case .ms-auto is unavailable
gap: 0.5rem; // Consistent spacing for appended actions
}

2 changes: 1 addition & 1 deletion app/assets/stylesheets/better_together/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
@use 'share';
@use 'sidebar_nav';
@use 'trix-extensions/richtext';
@use 'resource_toolbar';

// Styles that use the variables
.text-opposite-theme {
Expand Down Expand Up @@ -104,4 +105,3 @@
.spin-horizontal {
animation: flip 1s linear infinite;
}

71 changes: 71 additions & 0 deletions app/assets/stylesheets/better_together/conversations.scss
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,74 @@
overflow-y: auto;
}

/* Responsive tweaks for conversation header & participants */
/* Reduce header padding and font size on small screens */
@media (max-width: 991.98px) { // Bootstrap lg breakpoint
/* Reduce overall header vertical padding on small screens */
.card-header {
padding-top: 0.25rem !important;
padding-bottom: 0.25rem !important;
}

/* Make the title smaller when present */
.card-header h4 {
font-size: 1rem; /* smaller header */
margin: 0;
}

/* Allow header children to wrap so we can place participants on their own row */
.card-header {
flex-wrap: wrap;
}

/* Reduce padding on the back link (left arrow) to save vertical space */
.card-header > a.p-4 {
padding-top: 0.25rem !important;
padding-bottom: 0.25rem !important;
padding-left: 0.5rem !important;
padding-right: 0.5rem !important;
}

/* Reduce horizontal margin around the options dropdown icon */
.card-header .mx-4 {
margin-left: 0.5rem !important;
margin-right: 0.5rem !important;
}

/* Place the participants container on its own full-width row below the header
when an h4 title exists, but keep participant items inline (horizontal). */
.card-header h4 + .conversation-participants {
flex: 0 0 100% !important;
order: 3;
width: 100%;
display: flex !important;
flex-direction: row !important; /* keep items side-by-side */
align-items: center !important;
justify-content: space-evenly !important; /* distribute participants evenly */
gap: 0.5rem;
margin-top: 0.5rem;
overflow-x: auto;
}

/* Small spacing adjustments for individual participant blocks when moved below */
.card-header h4 + .conversation-participants .mention_person {
margin-right: 0.5rem !important;
padding-top: 0.15rem;
padding-bottom: 0.15rem;
}

/* Shrink avatar size for participants in the header row to reduce height */
.card-header h4 + .conversation-participants .mention_person .profile-image {
width: 32px !important;
height: 32px !important;
object-fit: cover;
}

/* Ensure the options (ellipsis) container hugs the right edge of the header */
.card-header > .align-self-center.mx-4 {
margin-left: auto !important;
order: 2; /* keep it on the top row before participants (participants are order:3) */
align-self: center !important;
}
}

56 changes: 51 additions & 5 deletions app/controllers/better_together/conversations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,23 @@ class ConversationsController < ApplicationController # rubocop:todo Metrics/Cla
helper_method :available_participants

def index
authorize @conversations
# Conversations list is prepared by set_conversations (before_action)
# Provide a blank conversation for the new-conversation form in the sidebar
@conversation = Conversation.new
authorize @conversation
end

def new
@conversation = Conversation.new
if params[:conversation].present?
conv_params = params.require(:conversation).permit(:title, participant_ids: [])
@conversation = Conversation.new(conv_params)
else
@conversation = Conversation.new
end

# Ensure nested message is available for the form (so users can create the first message inline)
@conversation.messages.build if @conversation.messages.empty?

authorize @conversation
end

Expand All @@ -32,6 +44,13 @@ def create # rubocop:todo Metrics/MethodLength, Metrics/AbcSize

@conversation = Conversation.new(filtered_params.merge(creator: helpers.current_person))

# If nested messages were provided, ensure the sender is set to the creator/current person
if @conversation.messages.any?
@conversation.messages.each do |m|
m.sender = helpers.current_person
end
end

authorize @conversation

if submitted_any && filtered_empty
Expand All @@ -52,7 +71,7 @@ def create # rubocop:todo Metrics/MethodLength, Metrics/AbcSize
end
end
elsif @conversation.save
@conversation.participants << helpers.current_person
@conversation.add_participant_safe(helpers.current_person)

respond_to do |format|
format.turbo_stream
Expand Down Expand Up @@ -203,20 +222,47 @@ def available_participants
end

def conversation_params
params.require(:conversation).permit(:title, participant_ids: [])
# Use model-defined permitted attributes so nested attributes composition stays DRY
params.require(:conversation).permit(*Conversation.permitted_attributes)
end

# Ensure participant_ids only include people the agent is allowed to message.
# If none remain, keep it empty; creator is always added after create.
def conversation_params_filtered # rubocop:todo Metrics/AbcSize
def conversation_params_filtered # rubocop:todo Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity
permitted = ConversationPolicy.new(helpers.current_user, Conversation.new).permitted_participants
permitted_ids = permitted.pluck(:id)
# Always allow the current person (creator/participant) to appear in the list
permitted_ids << helpers.current_person.id if helpers.current_person

cp = conversation_params.dup

# Filter participant_ids to only those the agent may message
if cp[:participant_ids].present?
cp[:participant_ids] = Array(cp[:participant_ids]).map(&:presence).compact & permitted_ids
end

# Protect nested messages on update: only allow creating messages via the create path.
# On update, permit edits only to existing messages that belong to the current person,
# and only allow their content (prevent sender_id spoofing or reassigning other people's messages).
if action_name == 'update' && cp[:messages_attributes].present?
safe_messages = Array(cp[:messages_attributes]).map do |m|
# handle ActionController::Parameters or Hash
attrs = m.respond_to?(:to_h) ? m.to_h : m
msg_id = attrs['id'] || attrs[:id]
next nil unless msg_id

msg = BetterTogether::Message.find_by(id: msg_id)
next nil unless msg && helpers.current_person && msg.sender_id == helpers.current_person.id

# Only allow content edits through this path
{ 'id' => msg_id, 'content' => attrs['content'] || attrs[:content] }
end.compact

# Replace messages_attributes with the vetted set (or nil if none)
cp[:messages_attributes] = safe_messages.presence
end

# On create, leave messages_attributes as-is so nested creation works; controller will set sender.
cp
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/better_together/messages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def set_conversation
end

def message_params
params.require(:message).permit(:content)
params.require(:message).permit(*BetterTogether::Message.permitted_attributes)
end

def notify_participants(message)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def set_navigation_area
end

def navigation_area_params
params.require(:navigation_area).permit(:name, :slug, :visible, :style, :protected)
params.require(:navigation_area).permit(*resource_class.permitted_attributes)
end

def resource_class
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,7 @@ def navigation_item
end

def navigation_item_params
params.require(:navigation_item).permit(
:navigation_area_id, :url, :icon, :position, :visible,
:item_type, :linkable_id, :parent_id, :route_name,
*resource_class.localized_attribute_list
)
params.require(:navigation_item).permit(*resource_class.permitted_attributes)
end

def resource_class
Expand Down
26 changes: 21 additions & 5 deletions app/helpers/better_together/form_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,32 +116,48 @@ def privacy_field(form:, klass:)
end

# rubocop:todo Metrics/MethodLength
def required_label(form_or_object, field, **options) # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
# Accepts an optional label_text override which, when provided, will be used
# instead of the model's human_attribute_name for the field. This is useful
# when the visible label needs to be different from the translated attribute
# name (for example: participant_ids -> "Add participants").
# rubocop:todo Metrics/PerceivedComplexity
def required_label(form_or_object, field, label_text: nil, **options) # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
# Determine if it's a form object or just an object
if form_or_object.respond_to?(:object)
object = form_or_object.object
label_text = object.class.human_attribute_name(field)
# Use provided label_text override if present, otherwise fall back to translation
label_text ||= object.class.human_attribute_name(field)
class_name = options.delete(:class_name)

# Use the provided class_name for validation check if present, otherwise use the object's class
else
object = form_or_object
label_text = object.class.human_attribute_name(field)
label_text ||= object.class.human_attribute_name(field)

# Use the provided class_name for validation check if present, otherwise use the object's class
end

klass = class_name ? class_name.constantize : object.class
is_required = class_field_required(klass, field)

# Append asterisk for required fields
label_text += " <span class='required-indicator'>*</span>" if is_required
# Append asterisk for required fields and attach tooltip to the asterisk
if is_required
tooltip_text = I18n.t('helpers.required_info', default: 'This field is required')
# Make the asterisk keyboard-focusable and allow the tooltip to be
# triggered by click as well as hover/focus so it works on mobile.
asterisk = content_tag(:span, '*', class: 'required-indicator', tabindex: 0, role: 'button',
data: { bs_toggle: 'tooltip', bs_trigger: 'hover focus click' },
title: tooltip_text)
label_text += " #{asterisk}"
end

if form_or_object.respond_to?(:label)
form_or_object.label(field, label_text.html_safe, options)
else
label_tag(field, label_text.html_safe, options)
end
end
# rubocop:enable Metrics/PerceivedComplexity
# rubocop:enable Metrics/MethodLength

# rubocop:todo Metrics/MethodLength
Expand Down
Loading
Loading