Skip to content

Remove state and mandatory columns from adjustments#279

Merged
jhawthorn merged 7 commits into
solidusio:masterfrom
jhawthorn:adjustment_cleanup
Aug 19, 2015
Merged

Remove state and mandatory columns from adjustments#279
jhawthorn merged 7 commits into
solidusio:masterfrom
jhawthorn:adjustment_cleanup

Conversation

@jhawthorn

Copy link
Copy Markdown
Contributor

This comes from a conversation with @jordan-brough a while ago.

The state machine on adjustment serves no purpose, as it allows changing between both states without restriction. open and close are also states which need an explanation, vs finalized, which we've used elsewhere in the codebase with the same meaning. open also isn't ideal as it overrides Kernel#open, which causes state machine warnings unless configured to override that method.

This also removes the mandatory column, which has been unused since the Spree 2.2 adjustment changes. I'm not sure how this fits in with our versioning requirements, this column has been "gone" forever in that it hasn't done anything, but I could see an old store still setting it (or setting it to false) which would now error instead of just doing nothing.

@jordan-brough

Copy link
Copy Markdown
Contributor

awesome. nice deprecation strategy on open & close.

since mandatory has been gone for a good while and since there's nothing we could map it to i kind of think as a developer i'd rather have it fail hard like you have it doing here so that any issues are quickly surfaced during testing. i suppose we could also deprecate it with no-op methods but what you have seems ok to me.

Comment thread core/app/models/spree/adjustment.rb Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it worth having finalize/unfinalize (private, non-bang versions) for open/close?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is.

@jhawthorn

Copy link
Copy Markdown
Contributor Author

Added non-bang versions of finalize! and unfinalize!

@jordan-brough

Copy link
Copy Markdown
Contributor

👍

@gmacdougall

Copy link
Copy Markdown
Member

I'm a big fan of this. My only concern is how cross DB compatible the execute in the migration would be, only because I don't write SQL like that often.

@jhawthorn

Copy link
Copy Markdown
Contributor Author

My only concern is how cross DB compatible the execute in the migration would be, only because I don't write SQL like that often.

Should be entirely standard SQL. Works on mysql, postgres and sqlite at least.

@gmacdougall

Copy link
Copy Markdown
Member

👍

jhawthorn added a commit that referenced this pull request Aug 19, 2015
Remove state and mandatory columns from adjustments
@jhawthorn jhawthorn merged commit e8da7cf into solidusio:master Aug 19, 2015
@jhawthorn jhawthorn deleted the adjustment_cleanup branch August 19, 2015 22:10
spaghetticode referenced this pull request in nebulab/solidus Mar 26, 2021
Move decorators to the correct folders and add EngineExtensions
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.

5 participants