WIP Feature/v 3336 rewrite klaviyo gem for spree 54#21
Open
Conversation
Contributor
WalkthroughRefactors Klaviyo integration into event-driven subscribers and services: removes controller/model decorators and a user concern, adds subscribers, jobs, and services, introduces metafield migration and rake task, updates presenters for optional helpers, adjusts jobs/services signatures, and refreshes tests and VCR fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Event Source
participant Subscriber as Klaviyo Subscriber
participant Job as ActiveJob
participant Service as SpreeKlaviyo Service
participant DB as Database
participant Klaviyo as Klaviyo API
Event->>Subscriber: emit (user.created / newsletter.created / order.canceled / shipment.shipped)
Subscriber->>Job: enqueue job (CreateOrUpdateProfile / MergeVisitorProfile / Subscribe / AnalyticsEvent)
Job->>Service: perform -> call service (klaviyo_integration, user/subscriber, guest_id/variant_id)
Service->>DB: read/update user or subscriber metafield (klaviyo_id, klaviyo.subscribed)
Service->>Klaviyo: API request (create/update/subscribe/unsubscribe/event)
Klaviyo-->>Service: response (success/failure)
Service-->>Job: return result
Job-->>Subscriber: completion (implicit)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
.rubocop.yml-20-21 (1)
20-21:⚠️ Potential issue | 🟡 MinorAdd a comment explaining why
Rails/RakeEnvironmentis disabled.The existing Rake task in
lib/tasks/migrations.rakeproperly declares its environment dependency, so disabling this cop appears intentional, likely in preparation for event-driven architecture changes. Document the rationale with an inline comment to clarify the decision for future maintainers.# DISABLED + # Rails/RakeEnvironment is disabled because [add reason here] Rails/RakeEnvironment: Enabled: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.rubocop.yml around lines 20 - 21, Add an inline comment above the Rails/RakeEnvironment rule in .rubocop.yml that explains why the cop is disabled: note that the existing Rake task in lib/tasks/migrations.rake already declares its environment dependency, this cop was intentionally turned off to avoid false positives and to accommodate planned event-driven architecture changes (or other project-specific reason), and include a short note pointing maintainers to lib/tasks/migrations.rake for the canonical declaration.spec/jobs/spree_klaviyo/unsubscribe_job_spec.rb-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorCorrect the example description to match behavior.
The example says it “calls” unsubscribe, but expectation asserts it does not call it.
Proposed fix
- it 'calls SpreeKlaviyo::Unsubscribe' do + it 'does not call SpreeKlaviyo::Unsubscribe' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/jobs/spree_klaviyo/unsubscribe_job_spec.rb` around lines 21 - 22, Update the example description to match the expectation: change the spec description string in the example containing SpreeKlaviyo::Unsubscribe so it reads that it "does not call SpreeKlaviyo::Unsubscribe" (or similar negative wording) instead of "calls SpreeKlaviyo::Unsubscribe" so the it block description aligns with the expect(...).to_not receive(:call) assertion.spec/jobs/spree_klaviyo/subscribe_job_spec.rb-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorFix misleading test description.
The test description says "calls SpreeKlaviyo::Subscribe" but the assertion verifies it does not call the service.
📝 Proposed fix
- it 'calls SpreeKlaviyo::Subscribe' do + it 'does not call SpreeKlaviyo::Subscribe' do expect(SpreeKlaviyo::Subscribe).to_not receive(:call)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/jobs/spree_klaviyo/subscribe_job_spec.rb` around lines 21 - 22, The example test description is misleading—update the spec description to match the assertion which expects SpreeKlaviyo::Subscribe to NOT be invoked; specifically change the it string in spec/jobs/spree_klaviyo/subscribe_job_spec.rb (the example referencing SpreeKlaviyo::Subscribe and the expect(SpreeKlaviyo::Subscribe).to_not receive(:call) assertion) to something like "does not call SpreeKlaviyo::Subscribe" so the description accurately reflects the test behavior.spec/jobs/spree_klaviyo/create_event_job_spec.rb-47-47 (1)
47-47:⚠️ Potential issue | 🟡 MinorFix confusing context description.
The context description
'without invalid parameters'is a double negative and contradicts the actual test scenarios (which test invalid/missing parameters). Consider renaming for clarity.📝 Proposed fix
- context 'without invalid parameters' do + context 'with invalid parameters' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/jobs/spree_klaviyo/create_event_job_spec.rb` at line 47, Rename the confusing RSpec context description in the spec/jobs/spree_klaviyo/create_event_job_spec.rb file: locate the context block currently labeled 'without invalid parameters' and change it to a clearer, positive description that matches the tests (e.g., 'with invalid parameters' or 'when parameters are invalid or missing') so the context text accurately reflects the test scenarios in that context.spec/jobs/spree_klaviyo/create_event_job_spec.rb-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorFix typo in test description.
The test description references
CreateOrUpdateProfilebut should referenceCreateEventto match what's actually being tested.📝 Proposed fix
- it 'calls SpreeKlaviyo::CreateOrUpdateProfile with correct arguments' do + it 'calls SpreeKlaviyo::CreateEvent with correct arguments' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/jobs/spree_klaviyo/create_event_job_spec.rb` at line 34, Update the example description string in the spec so it accurately reflects the behavior under test: replace the "it" description that currently says 'calls SpreeKlaviyo::CreateOrUpdateProfile with correct arguments' with 'calls SpreeKlaviyo::CreateEvent with correct arguments' in the create_event_job_spec (the test wrapping the CreateEvent job) so the description matches the class under test (SpreeKlaviyo::CreateEvent).spec/services/spree_klaviyo/fetch_and_store_user_klaviyo_id_spec.rb-26-34 (1)
26-34:⚠️ Potential issue | 🟡 MinorAdd assertion for
klaviyo_idpersistence.The service implementation updates
user.klaviyo_idon successful profile fetch (line 12 in the service), but the test only verifies the return value. Since the service name explicitly indicates "Store", this side effect should be tested.Suggested test addition
context 'when user has profile in Klaviyo' do let(:fetch_profile_result) { Spree::ServiceModule::Result.new(true, { data: [{ id: '123' }] }.to_json) } it { is_expected.to be_success } it 'returns the klaviyo_id' do expect(call.value).to eq('123') end + + it 'stores the klaviyo_id on the user' do + expect { call }.to change { user.reload.klaviyo_id }.from(nil).to('123') + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/spree_klaviyo/fetch_and_store_user_klaviyo_id_spec.rb` around lines 26 - 34, Add an assertion that the user's klaviyo_id is persisted after the service runs: after invoking the service (the existing call/subject), reload the user record (user.reload) and assert user.klaviyo_id == '123' to verify the side effect implemented in the service (the update to user.klaviyo_id on successful fetch).spec/services/spree_klaviyo/create_or_update_profile_spec.rb-22-26 (1)
22-26:⚠️ Potential issue | 🟡 MinorSame cassette typo here.
The cassette name
success_with_exisitng_profileshould besuccess_with_existing_profile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/spree_klaviyo/create_or_update_profile_spec.rb` around lines 22 - 26, Fix the VCR cassette name typo in the spec: update the VCR.use_cassette call inside the "it 'links an existing profile'" example so the cassette string is "success_with_existing_profile" (correcting the misspelling "success_with_exisitng_profile"); ensure the VCR.use_cassette invocation in that example matches the actual cassette filename so the test loads the recorded cassette.spec/models/spree/integrations/klaviyo_spec.rb-223-223 (1)
223-223:⚠️ Potential issue | 🟡 MinorTypo in stubbed value: "extarnal" should be "external".
While this doesn't affect test behavior, it could cause confusion when debugging.
🧹 Proposed fix
- allow_any_instance_of(SpreeKlaviyo::UserPresenter).to receive(:klaviyo_external_id).and_return('some-extarnal-id') + allow_any_instance_of(SpreeKlaviyo::UserPresenter).to receive(:klaviyo_external_id).and_return('some-external-id')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/spree/integrations/klaviyo_spec.rb` at line 223, Replace the misspelled stub value 'some-extarnal-id' with the correct spelling 'some-external-id' in the test that stubs SpreeKlaviyo::UserPresenter#klaviyo_external_id (the allow_any_instance_of(...).to receive(:klaviyo_external_id).and_return(...) line) so the fixture accurately reflects "external" and avoids confusion.spec/models/spree/integrations/klaviyo_spec.rb-158-158 (1)
158-158:⚠️ Potential issue | 🟡 MinorTypo: "quest" should be "guest".
🧹 Proposed fix
- context 'for a quest user' do + context 'for a guest user' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/spree/integrations/klaviyo_spec.rb` at line 158, Typo in the spec context description: change the context string "for a quest user" to "for a guest user" in the spec file spec/models/spree/integrations/klaviyo_spec.rb (look for the context block labeled 'for a quest user') so the description accurately reflects the test scenario.spec/services/spree_klaviyo/create_or_update_profile_spec.rb-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorRemove unused and broken
profile_datadefinition.
profile_datareferencescall_with_requestwhich is not defined anywhere in this file. This would raise aNameErrorif used. Since it's never referenced, it should be removed.🧹 Proposed fix
- let(:profile_data) { JSON.parse(call_with_request.value)['data'] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/spree_klaviyo/create_or_update_profile_spec.rb` at line 9, Remove the unused and broken let declaration by deleting the `let(:profile_data) { JSON.parse(call_with_request.value)['data'] }` line (it references undefined `call_with_request`); search the spec for any usages of `profile_data` to confirm none remain and update or remove those tests if found so the spec only defines and uses valid test helpers/fixtures.spec/services/spree_klaviyo/create_or_update_profile_spec.rb-33-33 (1)
33-33:⚠️ Potential issue | 🟡 MinorTypo in test description.
"assigned" should be "assigns".
🧹 Proposed fix
- it 'assigned klaviyo_id from newly created profile to user' do + it 'assigns klaviyo_id from newly created profile to user' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/spree_klaviyo/create_or_update_profile_spec.rb` at line 33, The example description string in the spec `it 'assigned klaviyo_id from newly created profile to user' do` contains a typo; update the `it` block description to use present tense (`assigns`) so it reads `it 'assigns klaviyo_id from newly created profile to user' do` to match RSpec style and correct grammar.spec/services/spree_klaviyo/create_or_update_profile_spec.rb-16-18 (1)
16-18:⚠️ Potential issue | 🟡 MinorTypo in VCR cassette name and redundant subject invocation.
- The cassette name has a typo:
exisitng_profile→existing_profile- Calling
calltwice within the sameitblock is redundant since the first invocation (insideexpect { call }) already executes the service.🧹 Proposed fix
- VCR.use_cassette('services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile') do - expect { call }.to change(user, :klaviyo_id).from(nil).to(id_from_response) - expect(call).to be_success + VCR.use_cassette('services/spree_klaviyo/create_or_update_profile/success_with_existing_profile') do + expect(call).to be_success + expect(user.klaviyo_id).to eq(id_from_response) endNote: This assumes VCR cassette files are also renamed accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/spree_klaviyo/create_or_update_profile_spec.rb` around lines 16 - 18, The VCR cassette name in the spec is misspelled and the service is invoked twice: update the cassette string passed to VCR.use_cassette from 'services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile' to 'services/spree_klaviyo/create_or_update_profile/success_with_existing_profile' (and rename the actual cassette file accordingly), and remove the redundant second call to the subject by replacing the second expect(call).to be_success with something that inspects the result of the first invocation (e.g., expect(subject).to be_success or capture the result from the first invocation) so the service is only executed once in the example using the subject `call`.
🧹 Nitpick comments (8)
Gemfile (1)
12-13: Consider capping the Spree version constraint.The current
>= 5.4.0.betaconstraints will resolve to future 5.5.x/6.x releases once they exist. While Spree 5.5 and 6.0 are not yet announced as of March 2026, adding an upper bound like< 5.5follows best practice for extension dependencies and avoids potential API drift in the future.📦 Optional fix
-gem 'spree', '>= 5.4.0.beta', spree_opts || {} -gem 'spree_admin', '>= 5.4.0.beta', spree_opts || {} +gem 'spree', '>= 5.4.0.beta', '< 5.5', spree_opts || {} +gem 'spree_admin', '>= 5.4.0.beta', '< 5.5', spree_opts || {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gemfile` around lines 12 - 13, Update the Gemfile gem version constraints for spree and spree_admin to prevent automatic upgrades to a future major/minor (e.g., 5.5); replace the current ">= 5.4.0.beta" constraints used in the spree and spree_admin gem declarations with a bounded constraint such as ">= 5.4.0.beta, < 5.5" so the Gemfile's spree and spree_admin declarations (the two gem 'spree' and gem 'spree_admin' lines) will not resolve to 5.5+ releases.config/initializers/spree.rb (1)
6-9: Prevent duplicate subscriber registration.Using
<<can register the same subscriber multiple times in re-init scenarios, causing duplicate event handling. Prefer set-style append.Proposed change
- Spree.subscribers << SpreeKlaviyo::OrderSubscriber - Spree.subscribers << SpreeKlaviyo::NewsletterSubscriber - Spree.subscribers << SpreeKlaviyo::ShipmentSubscriber - Spree.subscribers << SpreeKlaviyo::KlaviyoProfileSubscriber + Spree.subscribers |= [ + SpreeKlaviyo::OrderSubscriber, + SpreeKlaviyo::NewsletterSubscriber, + SpreeKlaviyo::ShipmentSubscriber, + SpreeKlaviyo::KlaviyoProfileSubscriber + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/initializers/spree.rb` around lines 6 - 9, Spree.subscribers is being appended with duplicate entries using the << operator; replace the repeated << calls by adding the four SpreeKlaviyo subscriber classes in a single set-style union operation (e.g., use a union/uniq pattern) so that Spree.subscribers includes SpreeKlaviyo::OrderSubscriber, SpreeKlaviyo::NewsletterSubscriber, SpreeKlaviyo::ShipmentSubscriber and SpreeKlaviyo::KlaviyoProfileSubscriber only once; ensure you reference Spree.subscribers and the four SpreeKlaviyo::* subscriber constants when implementing the change.spec/vcr/klaviyo/update_profile/success.yml (1)
10-10: Consider fixing fixture typo inexternal_idsample value.
some-extarnal-idappears to be a typo; replacing withsome-external-idimproves test fixture clarity.Also applies to: 76-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vcr/klaviyo/update_profile/success.yml` at line 10, Replace the misspelled fixture value for the external_id field in the VCR cassette so tests are clearer: find occurrences of "some-extarnal-id" in the spec/vcr/klaviyo/update_profile/success.yml fixture (and the other occurrence reported) and change them to "some-external-id" for the external_id sample value.app/jobs/spree_klaviyo/subscribe_job.rb (1)
4-6: Asymmetric error handling is intentional—consider documenting it.The job uses
findforklaviyo_integration(raisesRecordNotFound) butfind_byforsubscriber(silent return). Bothsubscribe_job.rbandunsubscribe_job.rbfollow this pattern consistently:
- Missing integration → job fails and retries (critical configuration issue)
- Missing subscriber → job silently exits (expected race condition, subscriber may be deleted between enqueue and perform)
This design is sound, but add a brief inline comment explaining why subscribers tolerate missing records while integrations do not.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/spree_klaviyo/subscribe_job.rb` around lines 4 - 6, Add a brief inline comment next to the asymmetric lookup in subscribe_job.rb (where ::Spree::Integrations::Klaviyo is fetched with find and Spree::NewsletterSubscriber is fetched with find_by) explaining that integration lookups intentionally raise (critical config -> retry) while subscriber lookups silently return because subscribers can be deleted between enqueue and perform; mirror the same explanatory comment in unsubscribe_job.rb near the corresponding find/find_by calls so future readers understand the intentional difference in error handling.app/services/spree_klaviyo/create_or_update_profile.rb (1)
6-6: The asymmetry inguest_idhandling appears intentional but should be documented.The early return on line 6 omits
guest_idbecause users with an existingklaviyo_idare already fully integrated and don't require guest-visitor merging. In contrast, lines 11 and 13 passguest_idwhen handling new profiles or guest-to-user merges (evident fromMergeVisitorProfileJobwhich explicitly passesguest_id: visitor_id). However, this design decision would benefit from an inline comment explaining why the early return doesn't need theguest_idparameter, particularly since the method signature accepts it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/spree_klaviyo/create_or_update_profile.rb` at line 6, Add a short inline comment above the early return in create_or_update_profile (the line calling klaviyo_integration.update_profile(user) when user.klaviyo_id.present?) that explains why guest_id is intentionally omitted for already-integrated users—e.g., that existing klaviyo_id users are fully linked and do not require guest-to-user merging so guest_id is only passed for new profiles or MergeVisitorProfileJob flows—so future readers understand the asymmetry between the early return and the guest_id-aware branches.spec/services/spree_klaviyo/unsubscribe_spec.rb (1)
29-35: Assert the failure path leaves the metafield unchanged.The failure example currently only checks the service result. Please also verify
klaviyo.subscribedstaystrue; otherwise a regression that flips local state on API failure would still pass.💡 Proposed fix
context 'when unsubscribe request fails' do let(:klaviyo_integration) { create(:klaviyo_integration, preferred_default_newsletter_list_id: 'invalid-list-id') } it 'returns failure' do VCR.use_cassette('services/spree_klaviyo/unsubscribe/failure') do expect(call).to be_failure end end + + it 'does not clear the subscribed metafield' do + VCR.use_cassette('services/spree_klaviyo/unsubscribe/failure') do + expect { call }.not_to change { + subscriber.get_metafield('klaviyo.subscribed').serialize_value + }.from(true) + end + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/spree_klaviyo/unsubscribe_spec.rb` around lines 29 - 35, Add an assertion in the failing-unsubscribe spec to ensure the local metafield isn't changed on API failure: after invoking the service via call inside the VCR.use_cassette block, reload the relevant integration/model and assert that klaviyo.subscribed remains true (e.g. using klaviyo.reload.subscribed or the integration record that holds the metafield). Target the existing let(:klaviyo_integration) setup and the call invocation so the spec verifies both the service result and that klaviyo.subscribed is unchanged.spec/models/spree/integrations/klaviyo_spec.rb (1)
119-124: Consider partial matching for error response.The assertion includes the
idfield which appears to be a correlation/request ID that may change if the VCR cassette is re-recorded. Consider matching only the stable fields likecode,detail, andstatus.💡 Alternative approach
it 'returns the correct error message' do - expect(response_value).to eq( - { 'errors' => [{ 'code' => 'variant_not_found', 'detail' => 'The variant in your relationship payload does not exist', - 'id' => 'd232453f-7418-46f7-b580-1630a30ea16d', 'links' => {}, 'meta' => {}, 'source' => { 'pointer' => '/data/relationships/variant/data/id' }, 'status' => 404, 'title' => 'The variant in your relationship payload does not exist' }] } - ) + error = response_value['errors'].first + expect(error).to include( + 'code' => 'variant_not_found', + 'status' => 404, + 'detail' => 'The variant in your relationship payload does not exist' + ) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/spree/integrations/klaviyo_spec.rb` around lines 119 - 124, The test example "returns the correct error message" is brittle because it asserts on a volatile `id`; update the expectation on `response_value` to perform a partial match instead: assert that `response_value` includes an 'errors' array whose first element matches a `hash_including` with stable keys `code`, `detail`, and `status` (and any other stable fields you want) rather than exact equality, using RSpec matchers (`include`, `hash_including`) to avoid depending on the changing `id` field.spec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rb (1)
40-68: Consider adding test coverage for address events.The subscriber also handles
address.createdandaddress.updatedevents (per the production code), but this spec only coversuser.createdanduser.updated. Adding tests for address events would ensure complete coverage of the subscriber's behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rb` around lines 40 - 68, Add specs for address events by creating an address (linked to the existing user) and building Spree::Event instances with name 'address.created' and 'address.updated' and payload { id: address.prefixed_id, store_id: store.id }, then call described_class.new.call(event) and assert the same job behavior as for user events: expect SpreeKlaviyo::CreateOrUpdateProfileJob.perform_later to be called with klaviyo_integration.id and address.user.id; add a context where the user's private_metadata contains 'klaviyo_visitor_id' to assert SpreeKlaviyo::MergeVisitorProfileJob is not enqueued; also add a context where klaviyo_integration.destroy! to assert no jobs are enqueued.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/jobs/spree_klaviyo/create_or_update_profile_job.rb`:
- Line 3: The job's perform method signature must remain backward-compatible so
already-enqueued jobs with the old arity don't fail; change the perform method
(perform) to accept a third optional argument (e.g., def
perform(klaviyo_integration_id, user_id, _legacy_arg = nil)) and ensure the
method implementation continues to use klaviyo_integration_id and user_id only,
ignoring the legacy param until rollout is complete, then remove the optional
arg once old jobs have drained.
In `@app/models/spree_klaviyo/user_decorator.rb`:
- Around line 9-10: In UserDecorator (method klaviyo_subscribed?) remove the
undefined .to_b call on the klaviyo_subscribed attribute and replace it with a
proper boolean cast; for example use
ActiveRecord::Type::Boolean.new.cast(klaviyo_subscribed) (or at minimum
!!klaviyo_subscribed) so klaviyo_subscribed? reliably returns a boolean without
invoking an undefined method.
In `@app/presenters/spree_klaviyo/product_presenter.rb`:
- Line 22: The presenter uses an unset instance variable `@store` when building
the storefront URL; change the call that passes store: `@store` to use the
initialized `@current_store` instead so spree_storefront_resource_url(`@product`,
store: `@current_store`) (locate this in product_presenter.rb where
spree_storefront_resource_url is invoked).
In `@app/presenters/spree_klaviyo/shipment_presenter.rb`:
- Line 39: The product URL is built without passing store context, which can
resolve to the wrong storefront in multi-store setups; update the call to
spree_storefront_resource_url used for shipped_item.variant.product so it is
invoked with the store: current_store keyword (i.e.,
spree_storefront_resource_url(shipped_item.variant.product, store:
current_store))—keep the respond_to? safeguard
(respond_to?(:spree_storefront_resource_url)) and pass the store: current_store
argument when present.
In `@app/presenters/spree_klaviyo/user_presenter.rb`:
- Around line 15-18: UserPresenter currently uses user_attributes and a
destructive merge! that assumes user responds to first_name/last_name and will
overwrite the explicit email param; change the presenter to tolerate
NewsletterSubscriber-like objects by: stop blindly merging user_attributes over
the existing hash (avoid merge! that overwrites email), only merge keys from
user_attributes that are present and non-nil (e.g., :first_name, :last_name,
:phone) and skip absent name/address fields, and keep the explicit email: param
intact; apply the same defensive merging approach to the other merges referenced
(address_attributes, try_klaviyo_id) so create_profile and update_profile accept
both User and NewsletterSubscriber inputs without losing caller-supplied email.
In `@app/services/spree_klaviyo/fetch_and_store_user_klaviyo_id.rb`:
- Around line 11-13: The code in FetchAndStoreUserKlaviyoId blindly accesses
JSON.parse(fetch_profile.value)['data'][0]['id'], which will raise if the
successful fetch returns an empty data array; instead parse fetch_profile.value
into a variable, check that parsed['data'] exists and is non-empty, and only
then call user.update!(klaviyo_id: parsed['data'][0]['id']) and return
success(user.klaviyo_id); if parsed['data'] is empty or missing, return the
appropriate not-found/failure result (e.g., not_found or failure(:not_found)) so
the caller can handle the missing profile case.
In `@app/services/spree_klaviyo/unsubscribe.rb`:
- Around line 8-9: The code calls klaviyo_integration.unsubscribe_user(email)
and then immediately sets subscriber.set_metafield('klaviyo.subscribed', false)
when result.success?, but result.success? only reflects job acceptance (202) not
completion; change this to mark a pending state instead of flipping subscribed
to false (e.g., set a metafield like 'klaviyo.unsub_pending' = true or
'klaviyo.subscribed' = 'pending_unsubscribe') in the unsubscribe flow (the block
that references klaviyo_integration.unsubscribe_user and
subscriber.set_metafield), and remove the immediate false set; then ensure the
actual false update happens only from the job completion handler (webhook or
polling) that processes the profile-subscription-bulk-delete-job result and
calls subscriber.set_metafield('klaviyo.subscribed', false) on confirmed
success.
In `@app/subscribers/spree_klaviyo/klaviyo_profile_subscriber.rb`:
- Around line 40-45: The find_user method currently assumes event.payload['id']
is always a user identifier which fails for address events; update find_user
(used by handle_profile_upsert) to detect address events (e.g., event.name or
event.payload keys) and when payload contains 'user_id' extract and use that
value (event.payload['user_id']) to look up the user instead of
event.payload['id'], or alternatively add a dedicated address event handler that
calls Spree.user_class.find_by_param with the user_id; ensure you reference and
update find_user, handle_profile_upsert, and the call to
Spree.user_class.find_by_param accordingly and add tests covering
address.created/address.updated payloads.
In `@app/subscribers/spree_klaviyo/newsletter_subscriber.rb`:
- Around line 5-18: The current rescue in handle(event)
(app/subscribers/spree_klaviyo/newsletter_subscriber.rb) reports errors but
swallows them; modify the handler so that after calling Rails.error.report(e,
context: { event_name: 'newsletter_subscriber.subscribed' }, source:
'spree_klaviyo') the exception is re-raised (raise e or just raise) so failures
from SpreeKlaviyo::SubscribeJob.perform_later or lookup steps bubble up to outer
error handling/retry policies; keep the existing reporting call and only add the
re-raise to the handle method.
In `@spec/support/vcr.rb`:
- Around line 18-29: The current custom matcher (config.register_request_matcher
:body) always replaces data.relationships.list.data.id with "PLACEHOLDER", which
masks the list ID difference needed by failure specs; change the matcher so it
only normalizes the list id when a cassette-level opt-in flag is set (e.g.,
check VCR.current_cassette&.metadata[:normalize_list_id] or cassette name
pattern) and otherwise leave the id intact; update failing-spec cassettes to
omit that metadata (or set normalize_list_id: false) so the failure-path
requests keep the real list id and the matcher will correctly detect the
difference.
In `@spec/vcr/klaviyo/create_event/order_completed_for_a_logged_user/success.yml`:
- Around line 8-19: The cassette spec
spec/vcr/klaviyo/create_event/order_completed_for_a_logged_user/success.yml is
recording stale order fields (value, subtotal, item_total, total, revenue = 0.0;
completed_at and time missing) despite items totaling 233.69; update the order
serialization or test setup that builds the "Order Completed" event so it
recalculates totals from the items array and sets completed_at/time to a valid
timestamp (ensure the serializer that emits the keys value, subtotal,
item_total, total, revenue, completed_at, time and the profile/event payload
uses those recalculated values), then re-run the test and re-record the cassette
so the fixture contains correct totals and timestamps.
In
`@spec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile.yml`:
- Around line 79-145: The cassette's PATCH payload and recorded response
diverge: the request sent external_id: 1 and anonymous_id: null but the response
shows external_id: "user.id" and anonymous_id populated; re-record the cassette
(spec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile.yml)
against a clean/new Klaviyo profile so the real POST/PATCH outcome is captured,
ensure the recorded request body contains the expected external_id and
anonymous_id values, and update the test assertions (the spec that currently
only checks klaviyo_id/status/email) to explicitly assert external_id and
anonymous_id were persisted.
---
Minor comments:
In @.rubocop.yml:
- Around line 20-21: Add an inline comment above the Rails/RakeEnvironment rule
in .rubocop.yml that explains why the cop is disabled: note that the existing
Rake task in lib/tasks/migrations.rake already declares its environment
dependency, this cop was intentionally turned off to avoid false positives and
to accommodate planned event-driven architecture changes (or other
project-specific reason), and include a short note pointing maintainers to
lib/tasks/migrations.rake for the canonical declaration.
In `@spec/jobs/spree_klaviyo/create_event_job_spec.rb`:
- Line 47: Rename the confusing RSpec context description in the
spec/jobs/spree_klaviyo/create_event_job_spec.rb file: locate the context block
currently labeled 'without invalid parameters' and change it to a clearer,
positive description that matches the tests (e.g., 'with invalid parameters' or
'when parameters are invalid or missing') so the context text accurately
reflects the test scenarios in that context.
- Line 34: Update the example description string in the spec so it accurately
reflects the behavior under test: replace the "it" description that currently
says 'calls SpreeKlaviyo::CreateOrUpdateProfile with correct arguments' with
'calls SpreeKlaviyo::CreateEvent with correct arguments' in the
create_event_job_spec (the test wrapping the CreateEvent job) so the description
matches the class under test (SpreeKlaviyo::CreateEvent).
In `@spec/jobs/spree_klaviyo/subscribe_job_spec.rb`:
- Around line 21-22: The example test description is misleading—update the spec
description to match the assertion which expects SpreeKlaviyo::Subscribe to NOT
be invoked; specifically change the it string in
spec/jobs/spree_klaviyo/subscribe_job_spec.rb (the example referencing
SpreeKlaviyo::Subscribe and the expect(SpreeKlaviyo::Subscribe).to_not
receive(:call) assertion) to something like "does not call
SpreeKlaviyo::Subscribe" so the description accurately reflects the test
behavior.
In `@spec/jobs/spree_klaviyo/unsubscribe_job_spec.rb`:
- Around line 21-22: Update the example description to match the expectation:
change the spec description string in the example containing
SpreeKlaviyo::Unsubscribe so it reads that it "does not call
SpreeKlaviyo::Unsubscribe" (or similar negative wording) instead of "calls
SpreeKlaviyo::Unsubscribe" so the it block description aligns with the
expect(...).to_not receive(:call) assertion.
In `@spec/models/spree/integrations/klaviyo_spec.rb`:
- Line 223: Replace the misspelled stub value 'some-extarnal-id' with the
correct spelling 'some-external-id' in the test that stubs
SpreeKlaviyo::UserPresenter#klaviyo_external_id (the
allow_any_instance_of(...).to receive(:klaviyo_external_id).and_return(...)
line) so the fixture accurately reflects "external" and avoids confusion.
- Line 158: Typo in the spec context description: change the context string "for
a quest user" to "for a guest user" in the spec file
spec/models/spree/integrations/klaviyo_spec.rb (look for the context block
labeled 'for a quest user') so the description accurately reflects the test
scenario.
In `@spec/services/spree_klaviyo/create_or_update_profile_spec.rb`:
- Around line 22-26: Fix the VCR cassette name typo in the spec: update the
VCR.use_cassette call inside the "it 'links an existing profile'" example so the
cassette string is "success_with_existing_profile" (correcting the misspelling
"success_with_exisitng_profile"); ensure the VCR.use_cassette invocation in that
example matches the actual cassette filename so the test loads the recorded
cassette.
- Line 9: Remove the unused and broken let declaration by deleting the
`let(:profile_data) { JSON.parse(call_with_request.value)['data'] }` line (it
references undefined `call_with_request`); search the spec for any usages of
`profile_data` to confirm none remain and update or remove those tests if found
so the spec only defines and uses valid test helpers/fixtures.
- Line 33: The example description string in the spec `it 'assigned klaviyo_id
from newly created profile to user' do` contains a typo; update the `it` block
description to use present tense (`assigns`) so it reads `it 'assigns klaviyo_id
from newly created profile to user' do` to match RSpec style and correct
grammar.
- Around line 16-18: The VCR cassette name in the spec is misspelled and the
service is invoked twice: update the cassette string passed to VCR.use_cassette
from
'services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile'
to
'services/spree_klaviyo/create_or_update_profile/success_with_existing_profile'
(and rename the actual cassette file accordingly), and remove the redundant
second call to the subject by replacing the second expect(call).to be_success
with something that inspects the result of the first invocation (e.g.,
expect(subject).to be_success or capture the result from the first invocation)
so the service is only executed once in the example using the subject `call`.
In `@spec/services/spree_klaviyo/fetch_and_store_user_klaviyo_id_spec.rb`:
- Around line 26-34: Add an assertion that the user's klaviyo_id is persisted
after the service runs: after invoking the service (the existing call/subject),
reload the user record (user.reload) and assert user.klaviyo_id == '123' to
verify the side effect implemented in the service (the update to user.klaviyo_id
on successful fetch).
---
Nitpick comments:
In `@app/jobs/spree_klaviyo/subscribe_job.rb`:
- Around line 4-6: Add a brief inline comment next to the asymmetric lookup in
subscribe_job.rb (where ::Spree::Integrations::Klaviyo is fetched with find and
Spree::NewsletterSubscriber is fetched with find_by) explaining that integration
lookups intentionally raise (critical config -> retry) while subscriber lookups
silently return because subscribers can be deleted between enqueue and perform;
mirror the same explanatory comment in unsubscribe_job.rb near the corresponding
find/find_by calls so future readers understand the intentional difference in
error handling.
In `@app/services/spree_klaviyo/create_or_update_profile.rb`:
- Line 6: Add a short inline comment above the early return in
create_or_update_profile (the line calling
klaviyo_integration.update_profile(user) when user.klaviyo_id.present?) that
explains why guest_id is intentionally omitted for already-integrated
users—e.g., that existing klaviyo_id users are fully linked and do not require
guest-to-user merging so guest_id is only passed for new profiles or
MergeVisitorProfileJob flows—so future readers understand the asymmetry between
the early return and the guest_id-aware branches.
In `@config/initializers/spree.rb`:
- Around line 6-9: Spree.subscribers is being appended with duplicate entries
using the << operator; replace the repeated << calls by adding the four
SpreeKlaviyo subscriber classes in a single set-style union operation (e.g., use
a union/uniq pattern) so that Spree.subscribers includes
SpreeKlaviyo::OrderSubscriber, SpreeKlaviyo::NewsletterSubscriber,
SpreeKlaviyo::ShipmentSubscriber and SpreeKlaviyo::KlaviyoProfileSubscriber only
once; ensure you reference Spree.subscribers and the four SpreeKlaviyo::*
subscriber constants when implementing the change.
In `@Gemfile`:
- Around line 12-13: Update the Gemfile gem version constraints for spree and
spree_admin to prevent automatic upgrades to a future major/minor (e.g., 5.5);
replace the current ">= 5.4.0.beta" constraints used in the spree and
spree_admin gem declarations with a bounded constraint such as ">= 5.4.0.beta, <
5.5" so the Gemfile's spree and spree_admin declarations (the two gem 'spree'
and gem 'spree_admin' lines) will not resolve to 5.5+ releases.
In `@spec/models/spree/integrations/klaviyo_spec.rb`:
- Around line 119-124: The test example "returns the correct error message" is
brittle because it asserts on a volatile `id`; update the expectation on
`response_value` to perform a partial match instead: assert that
`response_value` includes an 'errors' array whose first element matches a
`hash_including` with stable keys `code`, `detail`, and `status` (and any other
stable fields you want) rather than exact equality, using RSpec matchers
(`include`, `hash_including`) to avoid depending on the changing `id` field.
In `@spec/services/spree_klaviyo/unsubscribe_spec.rb`:
- Around line 29-35: Add an assertion in the failing-unsubscribe spec to ensure
the local metafield isn't changed on API failure: after invoking the service via
call inside the VCR.use_cassette block, reload the relevant integration/model
and assert that klaviyo.subscribed remains true (e.g. using
klaviyo.reload.subscribed or the integration record that holds the metafield).
Target the existing let(:klaviyo_integration) setup and the call invocation so
the spec verifies both the service result and that klaviyo.subscribed is
unchanged.
In `@spec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rb`:
- Around line 40-68: Add specs for address events by creating an address (linked
to the existing user) and building Spree::Event instances with name
'address.created' and 'address.updated' and payload { id: address.prefixed_id,
store_id: store.id }, then call described_class.new.call(event) and assert the
same job behavior as for user events: expect
SpreeKlaviyo::CreateOrUpdateProfileJob.perform_later to be called with
klaviyo_integration.id and address.user.id; add a context where the user's
private_metadata contains 'klaviyo_visitor_id' to assert
SpreeKlaviyo::MergeVisitorProfileJob is not enqueued; also add a context where
klaviyo_integration.destroy! to assert no jobs are enqueued.
In `@spec/vcr/klaviyo/update_profile/success.yml`:
- Line 10: Replace the misspelled fixture value for the external_id field in the
VCR cassette so tests are clearer: find occurrences of "some-extarnal-id" in the
spec/vcr/klaviyo/update_profile/success.yml fixture (and the other occurrence
reported) and change them to "some-external-id" for the external_id sample
value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d7e43db-6a9c-4343-bfac-df520d72f912
📒 Files selected for processing (88)
.rubocop.ymlGemfileapp/controllers/spree_klaviyo/addresses_controller_decorator.rbapp/controllers/spree_klaviyo/profile_controller_decorator.rbapp/controllers/spree_klaviyo/user_registrations_controller_decorator.rbapp/jobs/spree_klaviyo/create_back_in_stock_subscription_job.rbapp/jobs/spree_klaviyo/create_or_update_profile_job.rbapp/jobs/spree_klaviyo/fetch_profile_job.rbapp/jobs/spree_klaviyo/merge_visitor_profile_job.rbapp/jobs/spree_klaviyo/subscribe_job.rbapp/jobs/spree_klaviyo/unsubscribe_job.rbapp/models/concerns/spree_klaviyo/user_methods.rbapp/models/spree_klaviyo/order_decorator.rbapp/models/spree_klaviyo/shipment_handler_decorator.rbapp/models/spree_klaviyo/user_decorator.rbapp/presenters/spree_klaviyo/order_presenter.rbapp/presenters/spree_klaviyo/product_presenter.rbapp/presenters/spree_klaviyo/shipment_presenter.rbapp/presenters/spree_klaviyo/taxon_presenter.rbapp/presenters/spree_klaviyo/user_presenter.rbapp/services/spree_klaviyo/create_back_in_stock_subscription.rbapp/services/spree_klaviyo/create_event.rbapp/services/spree_klaviyo/create_or_update_profile.rbapp/services/spree_klaviyo/fetch_and_store_user_klaviyo_id.rbapp/services/spree_klaviyo/fetch_profile.rbapp/services/spree_klaviyo/subscribe.rbapp/services/spree_klaviyo/unsubscribe.rbapp/subscribers/spree_klaviyo/klaviyo_profile_subscriber.rbapp/subscribers/spree_klaviyo/newsletter_subscriber.rbapp/subscribers/spree_klaviyo/order_subscriber.rbapp/subscribers/spree_klaviyo/shipment_subscriber.rbbin/railsconfig/initializers/spree.rbdb/migrate/20260313113627_add_user_to_posts.rblib/spree_klaviyo.rblib/spree_klaviyo/engine.rblib/spree_klaviyo/metafield_migration.rblib/tasks/migrations.rakespec/jobs/spree_klaviyo/create_event_job_spec.rbspec/jobs/spree_klaviyo/create_or_update_profile_job_spec.rbspec/jobs/spree_klaviyo/merge_visitor_profile_job_spec.rbspec/jobs/spree_klaviyo/subscribe_job_spec.rbspec/jobs/spree_klaviyo/unsubscribe_job_spec.rbspec/models/spree/integrations/klaviyo_spec.rbspec/models/spree/order_spec.rbspec/models/spree/shipment_spec.rbspec/models/spree/user_spec.rbspec/services/spree_klaviyo/create_or_update_profile_spec.rbspec/services/spree_klaviyo/fetch_and_store_user_klaviyo_id_spec.rbspec/services/spree_klaviyo/fetch_profile_spec.rbspec/services/spree_klaviyo/subscribe_spec.rbspec/services/spree_klaviyo/unsubscribe_spec.rbspec/spec_helper.rbspec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rbspec/subscribers/spree_klaviyo/newsletter_subscriber_spec.rbspec/subscribers/spree_klaviyo/order_subscriber_spec.rbspec/subscribers/spree_klaviyo/shipment_subscriber_spec.rbspec/support/vcr.rbspec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_does_not_have_profile_in_klaviyo/creates_a_new_profile.ymlspec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_does_not_have_profile_in_klaviyo/with_a_guest_id/creates_a_new_profile_and_updates_it_with_the_guest_id.ymlspec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/links_an_existing_profile.ymlspec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/when_a_guest_id_is_provided/links_user_and_guest_profiles.ymlspec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/when_a_guest_id_is_provided/when_the_user_profile_already_exists/updates_the_user_profile_with_the_guest_id.ymlspec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/when_email_is_a_subaddress/links_an_existing_profile.ymlspec/vcr/klaviyo/can_connect/false/invalid_default_newsletter_list_id.ymlspec/vcr/klaviyo/can_connect/invalid_default_newsletter_list_id.ymlspec/vcr/klaviyo/can_connect/invalid_private_api_key.ymlspec/vcr/klaviyo/can_connect/success.ymlspec/vcr/klaviyo/create_back_in_stock_subscription/failure.ymlspec/vcr/klaviyo/create_back_in_stock_subscription/success.ymlspec/vcr/klaviyo/create_event/order_completed/success.ymlspec/vcr/klaviyo/create_event/order_completed_for_a_logged_user/failure.ymlspec/vcr/klaviyo/create_event/order_completed_for_a_logged_user/success.ymlspec/vcr/klaviyo/create_event/product_viewed_for_a_quest_user/success.ymlspec/vcr/klaviyo/create_profile/success.ymlspec/vcr/klaviyo/create_profile/success_with_additional_data.ymlspec/vcr/klaviyo/subscribe_user/failure.ymlspec/vcr/klaviyo/subscribe_user/success.ymlspec/vcr/klaviyo/unsubscribe_user/failure.ymlspec/vcr/klaviyo/unsubscribe_user/success.ymlspec/vcr/klaviyo/update_profile/failure.ymlspec/vcr/klaviyo/update_profile/success.ymlspec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile.ymlspec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_new_profile.ymlspec/vcr/services/spree_klaviyo/subscribe/failure.ymlspec/vcr/services/spree_klaviyo/subscribe/success.ymlspec/vcr/services/spree_klaviyo/unsubscribe/failure.ymlspec/vcr/services/spree_klaviyo/unsubscribe/success.yml
💤 Files with no reviewable changes (20)
- spec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/when_email_is_a_subaddress/links_an_existing_profile.yml
- spec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/links_an_existing_profile.yml
- spec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/when_a_guest_id_is_provided/when_the_user_profile_already_exists/updates_the_user_profile_with_the_guest_id.yml
- spec/vcr/klaviyo/create_event/order_completed/success.yml
- spec/models/spree/shipment_spec.rb
- spec/vcr/klaviyo/can_connect/false/invalid_default_newsletter_list_id.yml
- spec/services/spree_klaviyo/fetch_profile_spec.rb
- app/controllers/spree_klaviyo/profile_controller_decorator.rb
- app/jobs/spree_klaviyo/fetch_profile_job.rb
- spec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_does_not_have_profile_in_klaviyo/creates_a_new_profile.yml
- spec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_has_a_profile_in_Klaviyo/when_a_guest_id_is_provided/links_user_and_guest_profiles.yml
- app/controllers/spree_klaviyo/addresses_controller_decorator.rb
- spec/models/spree/order_spec.rb
- app/services/spree_klaviyo/create_event.rb
- app/services/spree_klaviyo/fetch_profile.rb
- app/controllers/spree_klaviyo/user_registrations_controller_decorator.rb
- app/models/spree_klaviyo/order_decorator.rb
- app/models/concerns/spree_klaviyo/user_methods.rb
- app/models/spree_klaviyo/shipment_handler_decorator.rb
- spec/vcr/SpreeKlaviyo_CreateOrUpdateProfile/when_klaviyo_integration_is_exists/when_user_does_not_have_profile_in_klaviyo/with_a_guest_id/creates_a_new_profile_and_updates_it_with_the_guest_id.yml
spec/vcr/klaviyo/create_event/order_completed_for_a_logged_user/success.yml
Outdated
Show resolved
Hide resolved
spec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile.yml
Outdated
Show resolved
Hide resolved
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/subscribers/spree_klaviyo/klaviyo_profile_subscriber.rb (1)
3-6: Reduce duplicated address-event literals to avoid drift.
address.created/address.updatedare repeated in both subscriptions and resolver logic. Consider extracting a constant and reusing it in both places for safer maintenance.♻️ Suggested refactor
module SpreeKlaviyo class KlaviyoProfileSubscriber < Spree::Subscriber + ADDRESS_EVENTS = %w[address.created address.updated].freeze + on 'user.created', :handle_user_created on 'user.updated', :handle_profile_upsert on 'address.created', :handle_profile_upsert on 'address.updated', :handle_profile_upsert @@ def find_user(event) param = - if event.name == 'address.created' || event.name == 'address.updated' + if ADDRESS_EVENTS.include?(event.name) event.payload['user_id'] else event.payload['id'] endAlso applies to: 40-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscribers/spree_klaviyo/klaviyo_profile_subscriber.rb` around lines 3 - 6, Extract the repeated address event string into a single constant (e.g. ADDRESS_EVENTS = %w[address.created address.updated]) and replace the literal occurrences in the subscriber subscription list (the on calls that currently use 'address.created' and 'address.updated') and in the resolver logic that checks those event names (used by handle_profile_upsert or any resolver methods) to reference that constant, ensuring both the subscription declarations and the resolver use the same source of truth; update any iteration or membership checks to use ADDRESS_EVENTS.include?(event) or iterate over ADDRESS_EVENTS where appropriate.app/presenters/spree_klaviyo/user_presenter.rb (1)
45-47: Consider adding a nil guard for defensive coding.
klaviyo_external_idcallsuser.iddirectly without a nil check. While this is safe in the current architecture (per learnings,UserPresenteris only instantiated with aSpree::Userobject), theinitializesignature acceptsuser: nilas a default, which could mislead future callers.♻️ Optional defensive fix
def klaviyo_external_id - user.id + user&.id end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/presenters/spree_klaviyo/user_presenter.rb` around lines 45 - 47, klaviyo_external_id should guard against a nil user to avoid NoMethodError if UserPresenter is ever instantiated without a user; update the method in UserPresenter (klaviyo_external_id) to return nil (or a sensible default) when user is nil by using a safe navigation or explicit nil check (e.g., use user&.id or return nil unless user) so the method never calls id on nil.spec/support/vcr.rb (1)
18-31: Redundant nestedVCR.configureblock.The
register_request_matchercall is already inside aVCR.configureblock (line 8), so nesting another one on line 18 is unnecessary. Flatten it to usecdirectly.♻️ Proposed fix
c.default_cassette_options = { allow_unused_http_interactions: false, record: :once, match_requests_on: [:method, :uri, :body] } - VCR.configure do |config| - config.register_request_matcher :body do |req1, req2| - normalize = ->(body) do - parsed = JSON.parse(body) - list_data = parsed.dig("data", "relationships", "list", "data") - list_data["id"] = "PLACEHOLDER" if list_data&.key?("id") - parsed - rescue JSON::ParserError - body - end - - normalize.call(req1.body) == normalize.call(req2.body) - end + c.register_request_matcher :body do |req1, req2| + normalize = ->(body) do + parsed = JSON.parse(body) + list_data = parsed.dig("data", "relationships", "list", "data") + list_data["id"] = "PLACEHOLDER" if list_data&.key?("id") + parsed + rescue JSON::ParserError + body + end + + normalize.call(req1.body) == normalize.call(req2.body) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/support/vcr.rb` around lines 18 - 31, Remove the redundant nested VCR.configure block and register the :body matcher directly in the existing outer configuration by calling register_request_matcher (:body) on the outer config variable (use the same config variable name used in the existing VCR.configure block, e.g., c or config) instead of wrapping another VCR.configure; keep the matcher implementation (normalize lambda, parsed.dig("data","relationships","list","data"), replacing list_data["id"] with "PLACEHOLDER", rescuing JSON::ParserError) and the equality check normalize.call(req1.body) == normalize.call(req2.body) intact, but move it up to the outer config scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/models/spree/integrations/klaviyo_spec.rb`:
- Line 163: Update the test context description string in the spec from "for a
quest user" to "for a guest user" to correct the typo; locate the context block
in spec/models/spree/integrations/klaviyo_spec.rb (the context 'for a quest
user' do) and change the string to "for a guest user" so the spec reads
correctly.
In `@spec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rb`:
- Around line 12-26: Add mutual-exclusion assertions so each spec not only
expects the intended job but also asserts the alternate job is not enqueued: in
the "enqueues CreateOrUpdateProfileJob" example (which calls
described_class.new.call(event) and expects
SpreeKlaviyo::CreateOrUpdateProfileJob.perform_later with klaviyo_integration.id
and user.id) also assert SpreeKlaviyo::MergeVisitorProfileJob does_not_receive
:perform_later; similarly, in the "enqueues MergeVisitorProfileJob" example
(where user.update_columns sets 'klaviyo_visitor_id' and you expect
SpreeKlaviyo::MergeVisitorProfileJob.perform_later with klaviyo_integration.id,
user.id, 'visitor-123') also assert SpreeKlaviyo::CreateOrUpdateProfileJob
does_not_receive :perform_later before calling described_class.new.call(event).
---
Nitpick comments:
In `@app/presenters/spree_klaviyo/user_presenter.rb`:
- Around line 45-47: klaviyo_external_id should guard against a nil user to
avoid NoMethodError if UserPresenter is ever instantiated without a user; update
the method in UserPresenter (klaviyo_external_id) to return nil (or a sensible
default) when user is nil by using a safe navigation or explicit nil check
(e.g., use user&.id or return nil unless user) so the method never calls id on
nil.
In `@app/subscribers/spree_klaviyo/klaviyo_profile_subscriber.rb`:
- Around line 3-6: Extract the repeated address event string into a single
constant (e.g. ADDRESS_EVENTS = %w[address.created address.updated]) and replace
the literal occurrences in the subscriber subscription list (the on calls that
currently use 'address.created' and 'address.updated') and in the resolver logic
that checks those event names (used by handle_profile_upsert or any resolver
methods) to reference that constant, ensuring both the subscription declarations
and the resolver use the same source of truth; update any iteration or
membership checks to use ADDRESS_EVENTS.include?(event) or iterate over
ADDRESS_EVENTS where appropriate.
In `@spec/support/vcr.rb`:
- Around line 18-31: Remove the redundant nested VCR.configure block and
register the :body matcher directly in the existing outer configuration by
calling register_request_matcher (:body) on the outer config variable (use the
same config variable name used in the existing VCR.configure block, e.g., c or
config) instead of wrapping another VCR.configure; keep the matcher
implementation (normalize lambda,
parsed.dig("data","relationships","list","data"), replacing list_data["id"] with
"PLACEHOLDER", rescuing JSON::ParserError) and the equality check
normalize.call(req1.body) == normalize.call(req2.body) intact, but move it up to
the outer config scope.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0bcd0d1-f681-40e1-ae66-4d7c556c2b52
📒 Files selected for processing (28)
.rubocop.ymlGemfileapp/models/spree/integrations/klaviyo.rbapp/models/spree_klaviyo/user_decorator.rbapp/presenters/spree_klaviyo/user_presenter.rbapp/services/spree_klaviyo/fetch_and_store_user_klaviyo_id.rbapp/subscribers/spree_klaviyo/klaviyo_profile_subscriber.rbapp/subscribers/spree_klaviyo/newsletter_subscriber.rblib/spree_klaviyo/factories.rblib/spree_klaviyo/testing_support/defaults.rblib/spree_klaviyo/testing_support/factories/klaviyo_integration.rbspec/models/spree/integrations/klaviyo_spec.rbspec/models/spree/user_spec.rbspec/spec_helper.rbspec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rbspec/subscribers/spree_klaviyo/newsletter_subscriber_spec.rbspec/support/vcr.rbspec/vcr/klaviyo/can_connect/invalid_private_api_key.ymlspec/vcr/klaviyo/can_connect/success.ymlspec/vcr/klaviyo/create_event/order_completed_for_a_logged_user/success.ymlspec/vcr/klaviyo/create_profile/success.ymlspec/vcr/klaviyo/create_profile/success_with_additional_data.ymlspec/vcr/klaviyo/update_profile/failure.ymlspec/vcr/klaviyo/update_profile/success.ymlspec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile.ymlspec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_new_profile.ymlspec/vcr/services/spree_klaviyo/subscribe/success.ymlspec/vcr/services/spree_klaviyo/unsubscribe/success.yml
✅ Files skipped from review due to trivial changes (13)
- .rubocop.yml
- lib/spree_klaviyo/factories.rb
- spec/vcr/services/spree_klaviyo/unsubscribe/success.yml
- spec/vcr/services/spree_klaviyo/subscribe/success.yml
- spec/vcr/klaviyo/update_profile/success.yml
- spec/vcr/klaviyo/can_connect/success.yml
- spec/vcr/klaviyo/update_profile/failure.yml
- spec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_exisitng_profile.yml
- spec/vcr/klaviyo/create_event/order_completed_for_a_logged_user/success.yml
- spec/vcr/services/spree_klaviyo/create_or_update_profile/success_with_new_profile.yml
- spec/vcr/klaviyo/create_profile/success.yml
- spec/vcr/klaviyo/create_profile/success_with_additional_data.yml
- Gemfile
🚧 Files skipped from review as they are similar to previous changes (4)
- spec/spec_helper.rb
- app/models/spree_klaviyo/user_decorator.rb
- spec/subscribers/spree_klaviyo/newsletter_subscriber_spec.rb
- spec/models/spree/user_spec.rb
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/models/spree_klaviyo/analytics_event_handler_spec.rb (1)
126-162:⚠️ Potential issue | 🟡 MinorOrphaned context block with no effect on tests.
The context
'when user is not present'(lines 127-135) defines a subject withuser: nil, but it contains no test examples. Theitblocks at lines 137-148 and 150-161 are outside this context, so they use the outer subject (which has a validuser).This explains why the tests assert
user.emailis passed—but if the intent was to test newsletter events when the user is absent, the tests should be nested inside the context.💡 Suggested fix: either remove the unused context or move tests inside it
If the tests should run with
user: nil:context 'when user is not present' do subject do described_class.new( user: nil, store: store, visitor_id: 'visitor_123' ) end - end it 'enqueues analytics event job for newsletter subscription' do expect { subject.handle_event('subscribed_to_newsletter', { email: 'test@example.com' }) }.to have_enqueued_job(SpreeKlaviyo::AnalyticsEventJob).with( klaviyo_integration.id, 'Subscribed to Newsletter', nil, nil, - user.email, + 'test@example.com', # or nil, depending on expected behavior 'visitor_123' ) end # ... similar for unsubscribe test + endOr remove the orphaned context if testing with a valid user is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/models/spree_klaviyo/analytics_event_handler_spec.rb` around lines 126 - 162, The context 'when user is not present' currently defines a subject (described_class.new with user: nil) but contains no examples, so the newsletter specs run against the outer subject with a real user; either delete that orphaned context block to avoid confusion, or move the two newsletter examples (the it blocks that call subject.handle_event with 'subscribed_to_newsletter' and 'unsubscribed_from_newsletter') inside that context so they use the nil-user subject; update any expected values (e.g., assertions on SpreeKlaviyo::AnalyticsEventJob payloads) accordingly if you choose to test the nil-user behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/subscribers/spree_klaviyo/newsletter_subscriber.rb`:
- Around line 18-20: In the rescue blocks inside NewsletterSubscriber
(specifically in the methods handling email subscription and
handle_email_unsubscription), replace re-raising with `raise e` with a bare
`raise` so the original exception backtrace is preserved; keep the existing
Rails.error.report call (Rails.error.report(e, context: { event_name: event.name
}, source: 'spree_klaviyo')) and then call bare `raise` to re-raise the same
exception without losing its original stack trace.
In `@spec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rb`:
- Around line 41-56: The test declares let(:klaviyo_visitor_id) but never
assigns it to the user, so the context title "'with visitor_id on both payload
and user'" is misleading; either attach the stored visitor id to the user (e.g.
set user.klaviyo_visitor_id or user.update to use klaviyo_visitor_id) so the
scenario truly has visitor_id on both payload and user before calling
described_class.new.call(event), or change the context description to reflect
only a payload visitor_id and remove the unused let; ensure the expectation on
SpreeKlaviyo::MergeVisitorProfileJob.perform_later still checks for
'payload-visitor'.
---
Outside diff comments:
In `@spec/models/spree_klaviyo/analytics_event_handler_spec.rb`:
- Around line 126-162: The context 'when user is not present' currently defines
a subject (described_class.new with user: nil) but contains no examples, so the
newsletter specs run against the outer subject with a real user; either delete
that orphaned context block to avoid confusion, or move the two newsletter
examples (the it blocks that call subject.handle_event with
'subscribed_to_newsletter' and 'unsubscribed_from_newsletter') inside that
context so they use the nil-user subject; update any expected values (e.g.,
assertions on SpreeKlaviyo::AnalyticsEventJob payloads) accordingly if you
choose to test the nil-user behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 353fdd0c-ba43-4d7b-b019-1fe051aaa5af
📒 Files selected for processing (22)
.gitignoreGemfileRakefileapp/controllers/spree_klaviyo/base_controller_decorator.rbapp/jobs/spree_klaviyo/unsubscribe_job.rbapp/models/spree_klaviyo/analytics_event_handler.rbapp/models/spree_klaviyo/current.rbapp/models/spree_klaviyo/publishable_decorator.rbapp/models/spree_klaviyo/user_decorator.rbapp/services/spree_klaviyo/unsubscribe.rbapp/subscribers/spree_klaviyo/klaviyo_profile_subscriber.rbapp/subscribers/spree_klaviyo/newsletter_subscriber.rbapp/subscribers/spree_klaviyo/order_subscriber.rbspec/jobs/spree_klaviyo/unsubscribe_job_spec.rbspec/models/spree/integrations/klaviyo_spec.rbspec/models/spree/user_spec.rbspec/models/spree_klaviyo/analytics_event_handler_spec.rbspec/services/spree_klaviyo/unsubscribe_spec.rbspec/spec_helper.rbspec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rbspec/subscribers/spree_klaviyo/newsletter_subscriber_spec.rbspec/subscribers/spree_klaviyo/order_subscriber_spec.rb
💤 Files with no reviewable changes (1)
- app/models/spree_klaviyo/analytics_event_handler.rb
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- Rakefile
- app/subscribers/spree_klaviyo/order_subscriber.rb
🚧 Files skipped from review as they are similar to previous changes (8)
- app/jobs/spree_klaviyo/unsubscribe_job.rb
- app/models/spree_klaviyo/user_decorator.rb
- spec/subscribers/spree_klaviyo/newsletter_subscriber_spec.rb
- spec/spec_helper.rb
- app/services/spree_klaviyo/unsubscribe.rb
- spec/models/spree/integrations/klaviyo_spec.rb
- Gemfile
- spec/jobs/spree_klaviyo/unsubscribe_job_spec.rb
Comment on lines
+18
to
+20
| rescue StandardError => e | ||
| Rails.error.report(e, context: { event_name: event.name }, source: 'spree_klaviyo') | ||
| raise e |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "newsletter_subscriber.rb" -path "*/spree_klaviyo/*"Repository: spree/spree_klaviyo
Length of output: 119
🏁 Script executed:
# Read the file to see the actual code at lines 18-20 and 31-33
if [ -f "app/subscribers/spree_klaviyo/newsletter_subscriber.rb" ]; then
cat -n "app/subscribers/spree_klaviyo/newsletter_subscriber.rb"
fiRepository: spree/spree_klaviyo
Length of output: 1806
🌐 Web query:
Ruby best practice bare raise vs raise e exception backtrace preservation
💡 Result:
In Ruby, when re-raising an exception after rescuing it (e.g., for logging), use bare raise (no arguments) as the best practice to preserve the original exception's backtrace. Using raise e (where e is the rescued exception) creates a new exception instance with a backtrace starting from the raise e line, losing the original backtrace. Bare raise re-raises the current exception from $!, preserving its class, message, and full original backtrace (from the initial raise point through the rescue). Example: begin raise "Original error" # Backtrace starts here rescue => e # Log e.message, e.backtrace raise # Preserves original backtrace end Output backtrace includes the original raise location. Contrast with: raise e # New backtrace: only from this raise line For wrapping with a new message while preserving backtrace, use raise "New message", e.backtrace or (Ruby 2.1+) raise "New message", cause: e. Official Kernel#raise docs confirm bare raise uses $! (current exception). Ruby exception docs and community sources (Stack Overflow) emphasize bare raise in rescue for backtrace preservation. Use fail instead of raise for initial failures (style preference), but raise for re-raising.
Citations:
- 1: https://www.rubydoc.info/stdlib/core/Kernel:raise
- 2: https://docs.ruby-lang.org/en/trunk/syntax/exceptions_rdoc.html
- 3: https://stackoverflow.com/questions/7250952/ruby-reraising-exception-w-additional-string-argument
- 4: https://stackoverflow.com/questions/23770924/reraise-same-exception-after-catching-an-exception-in-ruby/69652924
- 5: https://notes.musayev.com/20230723_4310/preserving-ruby-exception-backtrace
- 6: https://stackoverflow.com/questions/31937632/fail-vs-raise-in-ruby-should-we-really-believe-the-style-guide
- 7: https://docs.ruby-lang.org/en/3.2/Exception.html
- 8: https://docs.ruby-lang.org/en/trunk/Exception.html
- 9: https://docs.ruby-lang.org/en/3.1/Exception.html
Use bare raise to preserve the original exception backtrace.
Re-raising with raise e creates a new exception with a backtrace starting from that line, losing the original stack trace. Inside rescue blocks, use bare raise to preserve the full backtrace from the initial error point.
Suggested patch
rescue StandardError => e
Rails.error.report(e, context: { event_name: event.name }, source: 'spree_klaviyo')
- raise e
+ raise
endAlso applies to lines 31-33 in handle_email_unsubscription.
📝 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
| rescue StandardError => e | |
| Rails.error.report(e, context: { event_name: event.name }, source: 'spree_klaviyo') | |
| raise e | |
| rescue StandardError => e | |
| Rails.error.report(e, context: { event_name: event.name }, source: 'spree_klaviyo') | |
| raise | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/subscribers/spree_klaviyo/newsletter_subscriber.rb` around lines 18 - 20,
In the rescue blocks inside NewsletterSubscriber (specifically in the methods
handling email subscription and handle_email_unsubscription), replace re-raising
with `raise e` with a bare `raise` so the original exception backtrace is
preserved; keep the existing Rails.error.report call (Rails.error.report(e,
context: { event_name: event.name }, source: 'spree_klaviyo')) and then call
bare `raise` to re-raise the same exception without losing its original stack
trace.
Comment on lines
+41
to
+56
| context 'with visitor_id on both payload and user' do | ||
| let(:klaviyo_visitor_id) { 'stored-visitor' } | ||
| let(:event) do | ||
| Spree::Event.new( | ||
| name: 'user.created', | ||
| payload: { id: user.prefixed_id, 'visitor_id' => 'payload-visitor' }, | ||
| store_id: store.id | ||
| ) | ||
| end | ||
|
|
||
| it 'prefers payload visitor_id' do | ||
| expect(SpreeKlaviyo::MergeVisitorProfileJob).to receive(:perform_later) | ||
| .with(klaviyo_integration.id, user.id, 'payload-visitor') | ||
| described_class.new.call(event) | ||
| end | ||
| end |
Contributor
There was a problem hiding this comment.
Unused let and misleading test description.
The let(:klaviyo_visitor_id) on line 42 is defined but never used—the user is not configured with a stored visitor_id. The context description 'with visitor_id on both payload and user' is therefore misleading, as only the payload has a visitor_id.
Either update the test to actually set the visitor_id on the user, or simplify the description to match actual behavior.
💡 Suggested fix
If the intent is to test payload precedence over stored visitor_id:
context 'with visitor_id on both payload and user' do
- let(:klaviyo_visitor_id) { 'stored-visitor' }
let(:event) do
Spree::Event.new(
name: 'user.created',
payload: { id: user.prefixed_id, 'visitor_id' => 'payload-visitor' },
store_id: store.id
)
end
+
+ before do
+ # Set stored visitor_id on user (adjust based on actual storage mechanism)
+ user.update!(private_metadata: { 'klaviyo_visitor_id' => 'stored-visitor' })
+ end
it 'prefers payload visitor_id' doOr if the test is redundant with the previous context, consider removing it.
📝 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
| context 'with visitor_id on both payload and user' do | |
| let(:klaviyo_visitor_id) { 'stored-visitor' } | |
| let(:event) do | |
| Spree::Event.new( | |
| name: 'user.created', | |
| payload: { id: user.prefixed_id, 'visitor_id' => 'payload-visitor' }, | |
| store_id: store.id | |
| ) | |
| end | |
| it 'prefers payload visitor_id' do | |
| expect(SpreeKlaviyo::MergeVisitorProfileJob).to receive(:perform_later) | |
| .with(klaviyo_integration.id, user.id, 'payload-visitor') | |
| described_class.new.call(event) | |
| end | |
| end | |
| context 'with visitor_id on both payload and user' do | |
| let(:event) do | |
| Spree::Event.new( | |
| name: 'user.created', | |
| payload: { id: user.prefixed_id, 'visitor_id' => 'payload-visitor' }, | |
| store_id: store.id | |
| ) | |
| end | |
| before do | |
| # Set stored visitor_id on user (adjust based on actual storage mechanism) | |
| user.update!(private_metadata: { 'klaviyo_visitor_id' => 'stored-visitor' }) | |
| end | |
| it 'prefers payload visitor_id' do | |
| expect(SpreeKlaviyo::MergeVisitorProfileJob).to receive(:perform_later) | |
| .with(klaviyo_integration.id, user.id, 'payload-visitor') | |
| described_class.new.call(event) | |
| end | |
| end |
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 51-52: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(SpreeKlaviyo::MergeVisitorProfileJob).to receive(:perform_later)
.with(klaviyo_integration.id, user.id, 'payload-visitor')
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 51-52: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: receive(:perform_later)
.with(klaviyo_integration.id, user.id, 'payload-visitor')
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/subscribers/spree_klaviyo/klaviyo_profile_subscriber_spec.rb` around
lines 41 - 56, The test declares let(:klaviyo_visitor_id) but never assigns it
to the user, so the context title "'with visitor_id on both payload and user'"
is misleading; either attach the stored visitor id to the user (e.g. set
user.klaviyo_visitor_id or user.update to use klaviyo_visitor_id) so the
scenario truly has visitor_id on both payload and user before calling
described_class.new.call(event), or change the context description to reflect
only a payload visitor_id and remove the unused let; ensure the expectation on
SpreeKlaviyo::MergeVisitorProfileJob.perform_later still checks for
'payload-visitor'.
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
1 participant
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.
major changes 💪
other changes 🐥
next steps and ideas 👀
testing 🧪
Summary by CodeRabbit
Release Notes
New Features
Improvements