Remove default address dependency part 2 #2802
Merged
Conversation
This file contains 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
kennyadsl
changed the title
BenMorganIO
reviewed
Sep 21, 2018
| def perform! | ||
| def perform!(created_by: nil) | ||
| unless created_by | ||
| created_by = Spree.user_class.where(email: '[email protected]').first |
There was a problem hiding this comment.
Use find_by instead of where.first.
There was a problem hiding this comment.
Done and squashed changes into 3f33fa199e889251c517a60c4ac98be958248b4c
kennyadsl
force-pushed
the
gevann-remove-default-address-dependency
branch
3 times, most recently
from
8afe61d to
381a522
Compare
ericsaupe
approved these changes
Oct 17, 2018
There was a problem hiding this comment.
I like the consistency change from whodunnit to created_by. I don't really love the Spree.user_class.find_by(email: '[email protected]') calls as I suspect nearly all production stores do not have that user but since it is within the deprecation call I think it's a good middle ground until it can be removed after the deprecation.
Thanks!
Without a user with email address equal to '[email protected]', when a reimbursement is created a Spree::StoreCredit is not. This spec illustrates this behaviour.
Previously, reimbursement helpers did not take a parameter for the creator of the reimbursement. Instead, they relied on a hard-coded dependency on a user in the database existing with the email address '[email protected]'.
Reimbursements only work if there is a user with the email address '[email protected]', or if you explicitly pass the creator when creating a reimbursement. This commit adds to the testing set-up for this spec by mocking a signed in admin creating the reimbursement, who can be passed as the creator.
This is done to keep the code language uniform, since at the end we are setting the store credit attribute (association with user) named created_by: https://github.com/solidusio/solidus/blob/826d9288684cf8f275bf04ac144b9b0ee84da1b6/core/app/models/spree/store_credit.rb#L24
whodunnit was basically doing the same thing. It's better to have the same argument name for every method that is using it.
This applies only to some deprecated blocks and does not change the new, working logic. This means that if those methods were used by some store or extension, they will now don't have the creator set instead of having a specific user that could even be not present in the database, causing the creator to be nil anyway.
kennyadsl
force-pushed
the
gevann-remove-default-address-dependency
branch
from
381a522 to
05b6354
Compare
|
@ericsaupe @gmacdougall I've addressed the concern we talked about in the core team meeting. This should be fine now! |
ericsaupe
approved these changes
Nov 1, 2018
gmacdougall
approved these changes
Nov 7, 2018
tvdeyen
approved these changes
Nov 7, 2018
This was referenced Nov 14, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Continuation of #2686
Closes #2930
This PR continues the work of @gevann removing hard-coded dependency of having a specific user in the database to perform a set of operations related to reimbursements.
@tvdeyen this basically seconds your comments on that PR, let me know if it's enough!
I've also deprecated
whodunnitargument from some methods in favor of a more consistentcreated_by.