-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Report Orders and Distributors should display variant #12930
base: master
Are you sure you want to change the base?
Report Orders and Distributors should display variant #12930
Conversation
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 work, and good thinking to migrate the options data to make it seamless for users.
I have a concern about the migration, do you think it's worth adding a test? Or are you confident you've already manually checked all possibilities?
db/migrate/20241011071014_update_item_name_to_product_in_od_report.rb
Outdated
Show resolved
Hide resolved
…port.rb Co-authored-by: David Cook <david@redcliffs.net>
@dacook - I'd manually tested all the possible use cases for this migration, I thought it would run only once so wasn't sure if it was worth a spec that would add up to the pipeline each time the workflow runs. |
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.
Wonderful, thank you.
Yes it's hard to know exactly what to spec or not. It's still production code that will modify data on every server, so I thought it worth being certain about exactly what it will and won't do.
I agree we don't need the test running every time, and maybe it would be possible to configure a runner to only test migrations when they are added. But I don't know if there's an easy way to do that. For now, I think we just occasionally archive the migrations and specs.
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.
Looks good 👍
What? Why?
item_name
selected, won't be selected anymore. So to keep the selected column names the same in this case, a data migration has been added which changes the selected item_name to the product in all the existing selected columns that already had selected item_nameWhat should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):