Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for undefined method 'map' for nil #66

Merged
merged 4 commits into from
Jan 29, 2023
Merged

Fix for undefined method 'map' for nil #66

merged 4 commits into from
Jan 29, 2023

Conversation

ewan-jrpets
Copy link
Contributor

Hi, thanks for this awesome project!

I found a slight issue which caused a runtime error. We used the script creator to do a 'free gift over £x' deal, and ran into a slight but that happened seemly at random. Using Shopify's debug tools did not show any rhyme or reason for what could be causing it.

This PR is just a patch to return false instead of causing the script to panic.

- match would receive a nil line_item, causing `undefined method 'map' for nil`
@jgodson
Copy link
Owner

jgodson commented Nov 9, 2022

Hello, thanks for the PR! That is very strange, I wouldn't expect that to ever happen. The change would need to be here instead as the changed file is generated based on that. There's some information about how that works here.

However, I think since if this can happen here, it would be better to change this to guard all the qualifiers and do something like:

cart.line_items.send(@li_match_type) do |item|
  next false if item.nil?
  qualifier.match?(item)
end

@ewan-jrpets
Copy link
Contributor Author

Hi, sorry for the delay and thanks for the feedback, I've done the changes. It generated the script with the correct fix, but I have not been able to test it in production just yet. It seems to work when testing in a dev shop.

Copy link
Owner

@jgodson jgodson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small tweak needed, but looks good otherwise! Let me know how testing goes on your side and I will also give it a test before merging it but I think this should be good!.

ruby_scripts/common/campaign.rb Outdated Show resolved Hide resolved
@jgodson
Copy link
Owner

jgodson commented Jan 29, 2023

I tested this and it doesn't cause any issues with my test scripts, I've never hit the nil line item but this should take care of that. It will have an effect on the all items matching case since we're returning false, but I think that's probably fine since an error would have also caused the script not to work anyway. Going to merge it in. Thanks!

@jgodson jgodson merged commit 927adf8 into jgodson:master Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants