feat(merchants): add raw data import (csv) for merchants#1992
Open
xBlaz3kx wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds CSV-based merchant bulk-import: MerchantImport model, DB columns, API POST /api/v1/merchants (write scope) with header normalization/validation, web UI integration, tests, and API documentation. ChangesMerchant CSV Import Feature
Sequence DiagramsequenceDiagram
participant Client
participant API
participant MerchantsController
participant MerchantImport
participant Family
participant Database
participant Response
Client->>API: POST /api/v1/merchants (CSV file + X-Api-Key)
API->>MerchantsController: route to create
MerchantsController->>MerchantsController: authorize_scope!(:write)
MerchantsController->>MerchantsController: validate file (presence, size, MIME)
MerchantsController->>MerchantImport: new + generate_rows_from_csv(file)
MerchantImport->>MerchantImport: validate_required_headers! / normalize headers
MerchantsController->>MerchantImport: import!
MerchantImport->>Family: use current_family
loop for each row
MerchantImport->>Database: find_or_initialize_by(name) scoped to family
MerchantImport->>Database: save if new_record? (set color, website_url)
end
MerchantImport-->>MerchantsController: imported/skipped counts + merchants
MerchantsController->>Response: render JSON 201 created
Response-->>Client: MerchantImportResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xBlaz3kx
force-pushed
the
feat/import-merchants
branch
from
ced3078 to
3edc98c
Compare
xBlaz3kx
force-pushed
the
feat/import-merchants
branch
from
3edc98c to
ff5e83f
Compare
superagent-security
Bot
added
contributor:verified
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/api/v1/merchants_controller.rb`:
- Around line 121-124: The rescue block in MerchantsController currently returns
a generic 500 payload; update the render in the rescue StandardError => e
handler so the JSON uses the API v1 standard message format: message: "Error:
#{e.message}" (keep the existing Rails.logger.error call and status:
:internal_server_error), i.e., locate the rescue StandardError => e block in
app/controllers/api/v1/merchants_controller.rb (references: rescue StandardError
=> e, Rails.logger.error, render json) and change the message value to "Error:
#{e.message}" for consistency with API v1.
In `@app/models/merchant_import.rb`:
- Around line 54-63: The CSV import loop creates row records (csv_rows.each /
rows.create!) but never updates the MerchantImport.rows_count attribute, which
leaves configured?/publishable? incorrect; after inserting the rows (or after
the loop), set self.rows_count to the new count (e.g. rows.reload.count or
csv_rows.size) and persist it (use update_column(:rows_count, ...) or
update!(rows_count: ...) depending on whether you need callbacks/validations) so
configured? and publishable? reflect the actual number of rows.
- Around line 84-86: normalized_headers currently uses csv_headers.to_h which
silently overwrites duplicate normalized keys (e.g.,
normalize_header("website-url") == normalize_header("website url")); change
normalized_headers to iterate csv_headers and build the hash manually using
normalize_header(header) as the key, and if a normalized key is already present
collect the duplicates and raise the same validation error used by the base
Import (or add to import.errors) so duplicates are reported instead of silently
overwritten; update references to normalized_headers, csv_headers, and
normalize_header when implementing this check.
In `@app/views/family_merchants/index.html.erb`:
- Around line 9-14: The DS::Link invocation for the import button opens a full
page and breaks the modal turbo-frame flow; update the DS::Link.new call that
renders the import button (the existing call with text: t(".import") and href:
new_import_path(type: "MerchantImport")) to target the modal turbo frame by
adding the turbo frame attribute (e.g., data: { turbo_frame: "modal" } or the
component's supported frame/target option) so the new_import_path loads inside
the modal frame rather than replacing the whole page.
In `@app/views/imports/new.html.erb`:
- Around line 228-236: The render call for the import option passes raw Tailwind
palette classes (icon_bg_class: "bg-orange-500/5", icon_text_class:
"text-orange-500") which violates the design-system token rule; update the
partial invocation (render "imports/import_option" for MerchantImport) to use
the project's design tokens from sure-design-system.css (e.g., replace
icon_bg_class with the container/background token such as "bg-container" or the
equivalent token your system uses, and replace icon_text_class with a semantic
text token like "text-primary" or the agreed token) so the partial receives
semantic design tokens instead of raw palette classes.
In `@docs/api/merchants.md`:
- Around line 173-177: Update the API docs table row for HTTP status 403 so the
`error` value matches the actual API response: replace `forbidden` with
`insufficient_scope` in the 403 row (the table entry showing "403 | `forbidden`
| API key lacks the `write` scope.") so the documented error string aligns with
the API's `error: "insufficient_scope"`.
In `@spec/requests/api/v1/merchants_spec.rb`:
- Around line 75-85: Add a new rswag response block for response '403'
'forbidden' inside the POST /api/v1/merchants spec: create a response '403'
entry that uses the same ErrorResponse schema and simulates an authenticated
client that lacks the required write scope by setting the request auth
header/fixture used elsewhere (e.g., set the API key or token variable used for
auth such as let(:'X-Api-Key') or the token helper) to an API key/token with
read-only scope; ensure the block calls run_test! so the 403 scope-denied case
is documented and executed alongside the existing 401 and 422 cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 188156c6-19ce-4477-bd3b-371559b7f1b3
📒 Files selected for processing (18)
app/controllers/api/v1/merchants_controller.rbapp/controllers/imports_controller.rbapp/helpers/imports_helper.rbapp/models/import.rbapp/models/merchant_import.rbapp/views/family_merchants/index.html.erbapp/views/import/configurations/_merchant_import.html.erbapp/views/imports/new.html.erbconfig/locales/models/merchant_import/en.ymlconfig/locales/views/imports/en.ymlconfig/locales/views/merchants/en.ymlconfig/routes.rbdb/migrate/20260525202505_add_merchant_columns_to_import_rows.rbdb/schema.rbdocs/api/merchants.mdspec/requests/api/v1/merchants_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/merchants_controller_test.rb
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/controllers/api/v1/merchants_controller.rb (1)
121-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the API v1 standard 500 error message format.
At Line 123, returning a generic message diverges from the API v1 convention and prior controller behavior.
Proposed fix
- render json: { error: "internal_server_error", message: "An unexpected error occurred" }, + render json: { error: "internal_server_error", message: "Error: #{e.message}" }, status: :internal_server_errorBased on learnings: In
app/controllers/api/v1/**/*.rb, 500 responses should usemessage: "Error: #{e.message}"consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/api/v1/merchants_controller.rb` around lines 121 - 124, The rescue block in the API v1 Merchants controller is returning a generic 500 message; update the rescue StandardError => e handler (the block containing Rails.logger.error(... ) and the render json call) to follow API v1 convention by rendering json with message: "Error: #{e.message}" (keep status: :internal_server_error and existing Rails.logger.error call intact) so the response matches other controllers in app/controllers/api/v1/*.rb.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/merchant_import.rb`:
- Around line 89-100: The duplicate-detection loop in csv_headers should skip
blank normalized headers so trailing empty columns don't produce a duplicate
empty key; inside the csv_headers.each loop (where normalize_header(header) is
assigned to key and result/duplicates are updated), add a guard to next if
key.blank? (or key == ""), so you only populate result and mark duplicates for
non-blank keys before adding the :duplicate_columns error and raising
ActiveRecord::RecordInvalid.
---
Duplicate comments:
In `@app/controllers/api/v1/merchants_controller.rb`:
- Around line 121-124: The rescue block in the API v1 Merchants controller is
returning a generic 500 message; update the rescue StandardError => e handler
(the block containing Rails.logger.error(... ) and the render json call) to
follow API v1 convention by rendering json with message: "Error: #{e.message}"
(keep status: :internal_server_error and existing Rails.logger.error call
intact) so the response matches other controllers in
app/controllers/api/v1/*.rb.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc9e7909-64e1-47ea-b9ff-25ae43d3ef61
📒 Files selected for processing (3)
app/controllers/api/v1/merchants_controller.rbapp/models/merchant_import.rbconfig/locales/models/merchant_import/en.yml
Comment on lines
+89
to
+100
| csv_headers.each do |header| | ||
| key = normalize_header(header) | ||
| if result.key?(key) | ||
| duplicates << header | ||
| else | ||
| result[key] = header | ||
| end | ||
| end | ||
|
|
||
| if duplicates.any? | ||
| errors.add(:base, :duplicate_columns, columns: duplicates.join(", ")) | ||
| raise ActiveRecord::RecordInvalid.new(self) |
Contributor
There was a problem hiding this comment.
Ignore blank normalized headers in duplicate detection.
At Line 90-Line 92, blank headers normalize to an empty key and can trigger duplicate_columns on CSVs with trailing empty columns, causing false import failures.
Proposed fix
csv_headers.each do |header|
key = normalize_header(header)
+ next if key.blank?
+
if result.key?(key)
duplicates << header
else
result[key] = header
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| csv_headers.each do |header| | |
| key = normalize_header(header) | |
| if result.key?(key) | |
| duplicates << header | |
| else | |
| result[key] = header | |
| end | |
| end | |
| if duplicates.any? | |
| errors.add(:base, :duplicate_columns, columns: duplicates.join(", ")) | |
| raise ActiveRecord::RecordInvalid.new(self) | |
| csv_headers.each do |header| | |
| key = normalize_header(header) | |
| next if key.blank? | |
| if result.key?(key) | |
| duplicates << header | |
| else | |
| result[key] = header | |
| end | |
| end | |
| if duplicates.any? | |
| errors.add(:base, :duplicate_columns, columns: duplicates.join(", ")) | |
| raise ActiveRecord::RecordInvalid.new(self) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/merchant_import.rb` around lines 89 - 100, The duplicate-detection
loop in csv_headers should skip blank normalized headers so trailing empty
columns don't produce a duplicate empty key; inside the csv_headers.each loop
(where normalize_header(header) is assigned to key and result/duplicates are
updated), add a guard to next if key.blank? (or key == ""), so you only populate
result and mark duplicates for non-blank keys before adding the
:duplicate_columns error and raising ActiveRecord::RecordInvalid.
Collaborator
|
Good feature addition that follows the
API test coverage: Auth, scope, import, skip, missing file, missing column, and invalid file type are all tested. The only gap is the file-too-large case ( rswag spec: Uses Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation
Tests