feat: refactor admin user handling in clone store process to improve …

[kipsang01]

…vendor association logic

[qodo-code-review]

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

[willymwai]

/agentic_describe

[willymwai]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Unguarded admin_user_id queries ✓ Resolved 🐞 Bug ≡ Correctness

Description

link_admin_user_to_vendor! and existing_admin_vendor_link now query admin_user_id without
checking the column exists, even though the same methods explicitly treat admin_user_id as
optional. If spree_vendor_users exists but lacks admin_user_id, clone-store flows will raise SQL
errors (unknown column) and fail.

Code

app/controllers/spree/olitt/clone_store/clone_store_controller.rb[223]

+          vendor_user ||= Spree::VendorUser.find_by(vendor_id: vendor.id, admin_user_id: nil)

Evidence

Both controller and provisioner codebases already guard one admin_user_id lookup behind
column_names.include?('admin_user_id'), which indicates the column is not guaranteed. However,
newly added lookups in the same methods and in existing_admin_vendor_link reference
admin_user_id unconditionally, which will error if the column is absent.

app/controllers/spree/olitt/clone_store/clone_store_controller.rb[211-227]
app/controllers/spree/olitt/clone_store/clone_store_controller.rb[240-246]
app/services/spree/olitt/clone_store/clone_request_provisioner.rb[152-169]
app/services/spree/olitt/clone_store/clone_request_provisioner.rb[181-187]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`admin_user_id` is queried unconditionally in both `link_admin_user_to_vendor!` and `existing_admin_vendor_link`, but the surrounding code indicates `admin_user_id` is not always present (compat mode). This can crash clone-store flows with "unknown column" when `spree_vendor_users` exists without `admin_user_id`.

### Issue Context
You already guard one `admin_user_id` query with `Spree::VendorUser.column_names.include?('admin_user_id')`; the new nil-lookup and the `existing_admin_vendor_link` queries need the same protection.

### Fix Focus Areas
- app/controllers/spree/olitt/clone_store/clone_store_controller.rb[211-246]
- app/services/spree/olitt/clone_store/clone_request_provisioner.rb[152-187]

### Suggested fix
- In both files, wrap **all** queries referencing `admin_user_id` (including `admin_user_id: nil` and `where.not(admin_user_id: nil)`) behind the same `column_names.include?('admin_user_id')` guard.
- If the column is missing, return `nil` from `existing_admin_vendor_link` and skip `admin_user_id`-based lookups in `link_admin_user_to_vendor!` (fall back to legacy `user_id`-based behavior only).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Legacy VendorUser not backfilled 🐞 Bug ☼ Reliability

Description

When legacy_user is present and no VendorUser exists yet, the code now creates a VendorUser but no
longer assigns user_id, leaving the legacy user unlinked to the vendor via user_id. This changes
behavior within the same method which still prefers locating a VendorUser by user_id for a legacy
user.

Code

app/controllers/spree/olitt/clone_store/clone_store_controller.rb[R225-227]

          vendor_user ||= Spree::VendorUser.new(vendor_id: vendor.id)
-          vendor_user.user_id = legacy_user.id if legacy_user.present? && vendor_user.respond_to?(:user_id=) && vendor_user.user_id.blank?
          vendor_user.admin_user_id = admin_user.id if vendor_user.respond_to?(:admin_user_id=)
          vendor_user.save! if vendor_user.new_record? || vendor_user.changed?

Evidence

The method still attempts to locate a VendorUser row using vendor_id + user_id when a legacy
user exists, but the only assignment performed on a newly created VendorUser is admin_user_id;
there is no longer any backfill to set user_id on that new record.

app/controllers/spree/olitt/clone_store/clone_store_controller.rb[215-227]
app/services/spree/olitt/clone_store/clone_request_provisioner.rb[156-168]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When a `legacy_user` is provided and there is no existing `Spree::VendorUser` row, the method now creates a new VendorUser but does not set `user_id`. This leaves the legacy user unassociated via `user_id`, despite the method still prioritizing a `user_id`-based lookup.

### Issue Context
Previously, the code set `vendor_user.user_id = legacy_user.id` when possible. That assignment is now gone in both the controller and provisioner versions.

### Fix Focus Areas
- app/controllers/spree/olitt/clone_store/clone_store_controller.rb[211-228]
- app/services/spree/olitt/clone_store/clone_request_provisioner.rb[152-169]

### Suggested fix
- When `legacy_user.present?`, and the model responds to `user_id=` (and `vendor_user.user_id.blank?`), set `vendor_user.user_id = legacy_user.id` before saving.
- Keep the assignment conditional (as it was) to avoid breaking installations where `user_id` is not available/used.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Inconsistent resolve return type 🐞 Bug ⚙ Maintainability

Description

CloneStoreController#resolve_admin_user_for_vendor sometimes returns a 2-element array and
sometimes returns a single admin user object, making the method contract ambiguous. A caller that
doesn’t destructure the return value can receive an Array instead of a user and break downstream
operations.

Code

app/controllers/spree/olitt/clone_store/clone_store_controller.rb[R230-238]

+        def resolve_admin_user_for_vendor(vendor:, email:, password:, password_confirmation:, legacy_user: nil)
+          vendor_user = existing_admin_vendor_link(vendor)
+          if vendor_user.present? && vendor_user.admin_user_id.present?
+            admin_user = Spree.admin_user_class.find_by(id: vendor_user.admin_user_id)
+            return [admin_user, false] if admin_user.present?
+          end
+
+          find_or_create_admin_user(email, password, password_confirmation, legacy_user: legacy_user)
+        end

Evidence

The controller helper returns [admin_user, false] in the early return path, but the fall-through
returns find_or_create_admin_user(...), which returns an admin user object (not an array). This
makes the return type dependent on which branch is taken.

app/controllers/spree/olitt/clone_store/clone_store_controller.rb[176-192]
app/controllers/spree/olitt/clone_store/clone_store_controller.rb[230-238]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`resolve_admin_user_for_vendor` returns different types depending on control flow (Array vs user object). This is easy to misuse and creates hidden coupling to destructuring at call sites.

### Issue Context
In the controller, `find_or_create_admin_user` returns a user object, but `resolve_admin_user_for_vendor` sometimes returns `[user, false]`.

### Fix Focus Areas
- app/controllers/spree/olitt/clone_store/clone_store_controller.rb[230-238]

### Suggested fix
Pick one contract and enforce it:
- **Option A (simplest for controller):** always return just `admin_user` (change the early return to `return admin_user`). Update caller to `admin_user = resolve_admin_user_for_vendor(...)`.
- **Option B (align with provisioner):** always return `[admin_user, created_bool]` and update `find_or_create_admin_user` (or wrap its result) to provide the boolean consistently.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Review Summary by Qodo

Refactor admin user resolution for vendor associations

✨ Enhancement

Walkthroughs

Description

• Refactor admin user handling to check existing vendor-admin associations
• Extract new resolve_admin_user_for_vendor method for reusable logic
• Add existing_admin_vendor_link helper to find existing vendor-user links
• Improve vendor association by removing legacy user_id assignment logic

Diagram

flowchart LR
  A["handle_create_vendor/<br/>clone_request_provisioner"] -->|calls| B["resolve_admin_user_for_vendor"]
  B -->|checks| C["existing_admin_vendor_link"]
  C -->|queries| D["Spree::VendorUser"]
  B -->|fallback| E["find_or_create_admin_user"]
  B -->|returns| F["admin_user, created_flag"]
  F -->|used by| G["assign_vendor_role<br/>link_admin_user_to_vendor"]

Loading

File Changes

1. app/controllers/spree/olitt/clone_store/clone_store_controller.rb ✨ Enhancement +27/-2

Extract admin user resolution logic into dedicated methods

• Refactored handle_create_vendor to use new resolve_admin_user_for_vendor method
• Added resolve_admin_user_for_vendor method to check existing vendor-admin links before creating
 new users
• Added existing_admin_vendor_link helper method to query vendor-user associations
• Updated link_admin_user_to_vendor! to search for unlinked vendor users and removed legacy
 user_id assignment

app/controllers/spree/olitt/clone_store/clone_store_controller.rb

---

2. app/services/spree/olitt/clone_store/clone_request_provisioner.rb ✨ Enhancement +24/-4

Align provisioner with refactored admin user resolution

• Refactored call method to use new resolve_admin_user_for_vendor instead of direct
 find_or_create_admin_user
• Added resolve_admin_user_for_vendor method with vendor parameter to check existing associations
• Added existing_admin_vendor_link helper method for querying vendor-user relationships
• Updated link_admin_user_to_vendor! to handle unlinked vendor users and removed legacy user_id
 logic

app/services/spree/olitt/clone_store/clone_request_provisioner.rb

---