Fix: #2855: Cookie Overflow when trying to import CasaCase CSV #6456

alexmalik · 2025-07-17T07:58:13Z

What github issue is this PR for, if any?

Resolves #2855

What changed, and why?

The file containing failed import rows changed from being stored in the session as a csv string, to being stored in the filesystem in the tmp directory.

In the spec spec/system/index_spec the change from check "sms-opt-in-checkbox" to find("#sms-opt-in-checkbox", visible: true).check was due to needing to make capybara wait for the sms check box modal to appear before trying to check it.

How is this tested? 💖💪

This is tested with several spec files:

This PR adds a service object so there is a spec directly for that
There is a request spec to check the details around the session and the service
There is a system spec to check that the error modal appears
A spec is still required to check the possible error messages returned by the #read method of the service - the error messages become the content of the 'failed_rows.csv'

Screenshots please :)

Feelings gif

This refactors the FailedImportCsv service and its usage in ImportsController to improve readability, reduce hidden side effects, and simplify responsibilities. Changes include: - Renaming `csv_service` to `failed_csv_service` for clearer intent - Isolating expiry logic (`expired?`, `remove_csv`) from read flow - Adding structured logging for removals and size limits - Extracting filename generation to its own method - Reducing max allowed file size from 1MB to 250KB - Updating specs to focus on behavior and remove unnecessary expectations This cleanup lays the groundwork for better testability and safer future changes.

Adds system test coverage for importing volunteers and supervisors where the CSV input lacks `display_name` values, a known cause of cookie overflow and failure. Changes include: - New fixture CSVs with missing display_name columns for both roles - Additional system specs validating that import failures are surfaced properly via the "CSV Import Error" modal - Consolidated admin import tests under a scoped "as an admin" context - Minor improvements to test structure and readability This test ensures we detect regressions in import validation and prevents oversized session cookies from invalid user objects.

Copilot

Pull Request Overview

This PR fixes issue #2855 by resolving cookie overflow problems when importing large CasaCase CSV files. The solution replaces storing failed import rows in the session with a filesystem-based approach using temporary files.

Introduces a new FailedImportCsv service to manage failed import data in temporary files
Updates the imports controller to use the new service instead of session storage
Adds comprehensive test coverage for the new service and updated controller behavior

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
app/services/failed_import_csv.rb	New service for managing failed import CSV data with file storage, size limits, and expiration
app/controllers/imports_controller.rb	Updated to use FailedImportCsv service instead of session storage
spec/services/failed_import_csv_spec.rb	Comprehensive test coverage for the new service
spec/requests/imports_spec.rb	Updated request specs to test file-based storage integration
spec/system/imports/index_spec.rb	Added system tests for failed import modal display
spec/fixtures/files/*.csv	Test fixture files for scenarios with missing display names
config/locales/en.yml	Localization labels for import types

Comments suppressed due to low confidence (2)

spec/system/imports/index_spec.rb:15

The existing test contexts are not properly nested under the new 'as an admin' context. This creates inconsistent test structure where some tests are inside the admin context and others are not, potentially causing confusion about test scope.

  context "as an admin" do

app/controllers/imports_controller.rb:5

The before_action callback name failed_csv_service is confusing as it suggests it returns a service, but it actually sets an instance variable. Consider renaming to setup_failed_csv_service or initialize_failed_csv_service for clarity.

  before_action :failed_csv_service, only: [:create, :download_failed]

Copilot · 2025-07-21T19:49:37Z

spec/system/imports/index_spec.rb


-      check "sms-opt-in-checkbox"
-      click_button "sms-opt-in-continue-button"
+        find("#sms-opt-in-checkbox", visible: true).check


This change from check "sms-opt-in-checkbox" to find("#sms-opt-in-checkbox", visible: true).check appears unrelated to the main PR purpose of fixing cookie overflow. This type of change should be in a separate commit or explained in the PR description.

Suggested change

find("#sms-opt-in-checkbox", visible: true).check

check "sms-opt-in-checkbox"

Have responded to this and added the explanation for changing check "sms-opt-in-checkbox" to find("#sms-opt-in-checkbox", visible: true).check to the PR description

Copilot · 2025-07-21T19:49:37Z

app/services/failed_import_csv.rb

+  require "csv"
+


The require "csv" statement inside the class is unnecessary in Rails applications as CSV is part of the Ruby standard library and is typically auto-loaded. Consider removing this explicit require.

Suggested change

require "csv"

Have removed the require "csv" statement from the app/services/failed_import_csv.rb file

Copilot · 2025-07-21T19:49:37Z

app/services/failed_import_csv.rb

+  end
+
+  def filename
+    "failed_rows_userid_#{user.id}.csv"


The filename includes the user ID directly without sanitization. While user IDs are typically safe, consider using a more secure approach like hashing the user ID or adding additional validation to prevent potential path traversal issues.

Suggested change

"failed_rows_userid_#{user.id}.csv"

"failed_rows_userid_#{hashed_user_id}.csv"

This has been fixed and the user id has been hashed.

alexmalik · 2025-07-21T21:51:51Z

Thanks for the review, I will implement these changes

compwron · 2025-07-24T16:05:07Z

:)

…d#2855) - Refactor `FailedImportCsv` to `FailedImportCsvService` for clarity and extensibility - Use hashed user ID (short_hash) in CSV failure filenames to increase security - Centralize filepath generation via `csv_filepath` on the service - Update fallback messaging for expired or missing CSVs - Refactor specs to use new service and path generation

alexmalik · 2025-07-27T21:08:49Z

I've done my best, and this is ready for another review

alexmalik added 3 commits July 10, 2025 14:42

fix: rubyforgood#2855 cookie overflow when importing

588dc0e

alexmalik requested review from compwron, FireLemons and elasticspoon as code owners July 17, 2025 07:58

github-actions bot added ruby Pull requests that update Ruby code Tests! 🎉💖👏 labels Jul 17, 2025

alexmalik mentioned this pull request Jul 17, 2025

[BUG] Cookie Overflow when trying to import CasaCase CSV #2855

Open

compwron requested a review from Copilot July 21, 2025 19:48

compwron previously approved these changes Jul 21, 2025

View reviewed changes

Copilot AI reviewed Jul 21, 2025

View reviewed changes

alexmalik marked this pull request as draft July 21, 2025 21:52

alexmalik dismissed compwron’s stale review via b60b4cc July 27, 2025 16:17

alexmalik marked this pull request as ready for review July 27, 2025 17:58

style: align with quotes and braces formatting rules

8ed5b2f

alexmalik force-pushed the fix-bug-2855-cookie-overflow branch from dd6fab5 to 8ed5b2f Compare July 27, 2025 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix: #2855: Cookie Overflow when trying to import CasaCase CSV #6456

Fix: #2855: Cookie Overflow when trying to import CasaCase CSV #6456

Uh oh!

alexmalik commented Jul 17, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 21, 2025

Uh oh!

alexmalik Jul 23, 2025

Uh oh!

Copilot AI Jul 21, 2025

Uh oh!

alexmalik Jul 23, 2025

Uh oh!

Copilot AI Jul 21, 2025

Uh oh!

alexmalik Jul 23, 2025

Uh oh!

alexmalik commented Jul 21, 2025

Uh oh!

compwron commented Jul 24, 2025

Uh oh!

alexmalik commented Jul 27, 2025

Uh oh!

Uh oh!

	find("#sms-opt-in-checkbox", visible: true).check
	check "sms-opt-in-checkbox"

	"failed_rows_userid_#{user.id}.csv"
	"failed_rows_userid_#{hashed_user_id}.csv"

Uh oh!

Fix: #2855: Cookie Overflow when trying to import CasaCase CSV #6456

Are you sure you want to change the base?

Fix: #2855: Cookie Overflow when trying to import CasaCase CSV #6456

Uh oh!

Conversation

alexmalik commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What github issue is this PR for, if any?

What changed, and why?

How is this tested? 💖💪

Screenshots please :)

Feelings gif

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

alexmalik Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

alexmalik Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

alexmalik Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

alexmalik commented Jul 21, 2025

Uh oh!

compwron commented Jul 24, 2025

Uh oh!

alexmalik commented Jul 27, 2025

Uh oh!

Uh oh!

alexmalik commented Jul 17, 2025 •

edited

Loading