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.
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
allow importing cancelled date, and fix handling of end_date #278
allow importing cancelled date, and fix handling of end_date #278
Changes from 1 commit
b04cdad
e8b0a82
4bba344
c7bdef2
f37a34c
3610d97
fbe2b7c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this switch statement purposefully doesn't have break statements and it is expected to flow through the lower cases. eg cancelled being the top case means that the cancelled date is also validated against the end date, next payment and trial end checks. ie the cancelled date must occur after the next payment date (which is a condition under the end date case).
I've never been a fan of this code though as it's super confusing, hard to follow and is not intuitive at all. I wonder if the csv importer would benefit just not validating the dates relationships at all? 🤔
It calls
$subscription->update_dates()
which validates them anyway and throws exceptions if they don't pass the validation rules.The csv importer/exporter calls update_dates in a try catch block so would be able to catch those exceptions and handle them appropriately anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realised that removing this section would essentially break an important part of test mode as folks would use test mode to verify that the dates are valid and
update_dates()
isn't called while in test mode. 🤔hmm. How important were these changes for the goal of this PR? It seems from the PR description that you were mostly set on allowing users to set cancelled dates and for the end date issue with regard to when the status is set.
So, can these proposed changes be removed from this PR and we can look into doing a bit more of an overhaul of the validation of dates at a later time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@james-allan you're right—these changes are a bit overzealous. I've reduced their scope to only add validation for checking
cancelled_date
vs.end_date
, so the behavior for validating other dates should be intact.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the data variable (the csv row) might assist with folks who have custom data that they would like to set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea—added in 4bba344