Do not allow prices with nil amount #3987
Conversation
core/db/migrate/20210312060857_delete_prices_with_nil_amount.rb
Outdated
Show resolved
Hide resolved
waiting-for-dev
force-pushed
the
waiting-for-dev/dont_allow_nil_amount
branch
from
041d7c9
to
3eb9fd4
Compare
There was a problem hiding this comment.
@waiting-for-dev great work, I think it makes sense. I'd move the data migration into a rake task (something similar to what we did for 2.11).
Please also take a look at the specs against older versions of Rails in the CI.
waiting-for-dev
force-pushed
the
waiting-for-dev/dont_allow_nil_amount
branch
from
3eb9fd4
to
43dfcb2
Compare
Yeah, I agree it's a better solution. I did it in a task named matching 3.0 version. Please, tell me if I should address another version.
Hmm.. all the failures reference a
|
|
@waiting-for-dev I think the CI errors are due to the way migration has been defined: you are using |
This logic will help in an upcoming work that will copy all prices from a master variant over its children variant (thus enforcing that no "empty" prices are created). This overtolerance was introduced in 58ec5cd. That commit introduced the `Spree::Price` model to allow dealing with multiple currencies. It allowed `amount` to be `nil` for master variants, variants with no master variant, and variants with a master variant with no price. AFAIK, these three scenarios are no considered by our business logic. In fact, the logic was removed 5 days later in 9d563e3. 58ec5cd also allowed for `amount` to be `nil` in any other case, but then it copied it from the master variant: solidusio@9d563e3#diff-c083f45179b62dc8c47e4fdcbf7ee31da80552100fc0258238817086e46e2925L35 However, this was also done in the controller layer and it's still in place: https://github.com/solidusio/solidus/blob/3cc2d3e492b0aa6c84ac66814db26bea4f17d2e0/backend/app/controllers/spree/admin/variants_controller.rb#L19
waiting-for-dev
force-pushed
the
waiting-for-dev/dont_allow_nil_amount
branch
from
43dfcb2
to
286d0e3
Compare
Thanks @kennyadsl , it seems everything is fine now 👍 |
There was a problem hiding this comment.
@waiting-for-dev thanks for this change, I think it makes total sense. I left a few comments on renaming the rake task, I believe there's a small typo in the name that may be worth addressing.
core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb
Outdated
Show resolved
Hide resolved
Thanks for reviewing it @spaghetticode The typo is fixed now 👍 |
|
@waiting-for-dev thank you! I think the spec file needs renaming: |
waiting-for-dev
force-pushed
the
waiting-for-dev/dont_allow_nil_amount
branch
from
dff7700
to
2982058
Compare
🤦 thanks @spaghetticode , fixed 🙈 |
kennyadsl
added
changelog:solidus_core
… generator Having both an `upgrade` task and an `update` generator is confusing. On top of that, it's better to have actionable items, which can be undesirable in some situations, the most explicit as possible. For this reason, we leave in place the `update` generator, which is just a safe path to update preferences, and remove the `upgrade` task. Instead, we add a generic message into the `update` generator pointing to the detailed instructions in the Changelog. In particular, at this point, the `update` task was calling to a single task introduced in solidusio#3987. We still keep the task reachable by end-users, but we rename it slightly to be more friendly.
… generator Having both an `upgrade` task and an `update` generator is confusing. On top of that, it's better to have actionable items, which can be undesirable in some situations, the most explicit as possible. For this reason, we leave in place the `update` generator, which is just a safe path to update preferences, and remove the `upgrade` task. Instead, we add a generic message into the `update` generator pointing to the detailed instructions in the Changelog. In particular, at this point, the `update` task was calling to a single task introduced in solidusio#3987. We still keep the task reachable by end-users, but we rename it slightly to be more friendly.
… generator Having both an `upgrade` task and an `update` generator is confusing. On top of that, it's better to have actionable items, which can be undesirable in some situations, the most explicit as possible. For this reason, we leave in place the `update` generator, which is just a safe path to update preferences, and remove the `upgrade` task. Instead, we add a generic message into the `update` generator pointing to the detailed instructions in the Changelog. In particular, at this point, the `update` task was calling to a single task introduced in solidusio#3987. We still keep the task reachable by end-users, but we rename it slightly to be more friendly.
… generator Having both an `upgrade` task and an `update` generator is confusing. On top of that, it's better to have actionable items, which can be undesirable in some situations, the most explicit as possible. For this reason, we leave in place the `update` generator, which is just a safe path to update preferences, and remove the `upgrade` task. Instead, we add a generic message into the `update` generator pointing to the detailed instructions in the Changelog. In particular, at this point, the `update` task was calling to a single task introduced in solidusio#3987. We still keep the task reachable by end-users, but we rename it slightly to be more friendly.
… generator Having both an `upgrade` task and an `update` generator is confusing. On top of that, it's better to have actionable items, which can be undesirable in some situations, the most explicit as possible. For this reason, we leave in place the `update` generator, which is just a safe path to update preferences, and remove the `upgrade` task. Instead, we add a generic message into the `update` generator pointing to the detailed instructions in the Changelog. In particular, at this point, the `update` task was calling to a single task introduced in solidusio#3987. We still keep the task reachable by end-users, but we rename it slightly to be more friendly.
Description
This overtolerance was introduced in 58ec5cd. That commit introduced
the
Spree::Pricemodel to allow dealing with multiple currencies. Itallowed
amountto benilfor master variants, variants with nomaster variant, and variants with a master variant with no price. AFAIK,
these three scenarios are no considered by our business logic.
In fact, the logic was removed 5 days later in 9d563e3.
58ec5cd also allowed for
amountto benilin any other case, butthen it copied it from the master variant:
9d563e3#diff-c083f45179b62dc8c47e4fdcbf7ee31da80552100fc0258238817086e46e2925L35
However, this was also done in the controller layer and it's still in
place:
solidus/backend/app/controllers/spree/admin/variants_controller.rb
Line 19 in 3cc2d3e
Please, tell me if you'd prefer a less aggressive migration process.
Checklist: