AASM column attribute direct assignment restriction #53

Closed
unabl4 opened this Issue Jan 17, 2013 · 12 comments

Comments

Projects
None yet
4 participants
@unabl4

unabl4 commented Jan 17, 2013

Just to make the main idea of th state machine more clear I have a proposal to restrict the aasm_column setter at all, because it barely makes any sense, if we have events to transition from one state to another. This way it would have been very strict and clean, which is good - we won't need to think if we accidently assigned a new state, which could be very unwanted.

So, basically, it would look something like this (exampe):

@user = User.new
@user.state # sleeping (Why so ? Please see my pull request for this one: alto#9)
@user.state = :run # Some TransitionException exception thrown - "Direct assignment is not allowed"
@user.sleeping? # true. No state change.
@user.run! # => true. Changing states only by using events
@user.running? # true. New state is active

I think it makes more sense to do it that way.

@ghost ghost assigned alto Jan 17, 2013

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto Jan 18, 2013

Member

From what I understand you want to use the bang method to force overrwriting the current state, even if your own rules (transitions) say you shouldn't. Is that right?

Member

alto commented Jan 18, 2013

From what I understand you want to use the bang method to force overrwriting the current state, even if your own rules (transitions) say you shouldn't. Is that right?

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto Jan 18, 2013

Member

We currently have three suggestions how to use the bang method:

  1. immediately save the state to the database (that's currently in use)
  2. throw an exception if the state transition fails
  3. force the requested state

In fact the current solution (1) doesn't add any extra value to classes which are not persisted (using ActiveRecord or Mongoid). All three solutions have their relevance and make sense to me. That's a tough decision.

Does anyone have any good arguments for or against one of them?

Member

alto commented Jan 18, 2013

We currently have three suggestions how to use the bang method:

  1. immediately save the state to the database (that's currently in use)
  2. throw an exception if the state transition fails
  3. force the requested state

In fact the current solution (1) doesn't add any extra value to classes which are not persisted (using ActiveRecord or Mongoid). All three solutions have their relevance and make sense to me. That's a tough decision.

Does anyone have any good arguments for or against one of them?

@unabl4

This comment has been minimized.

Show comment
Hide comment
@unabl4

unabl4 Jan 18, 2013

I just had an idea, to use only the event method (bang or ordinary method) calls: run!, sleep!, etc just to switch from state to state. And to restrict direct assignment.

By the way. Using NON-bang methods doesn't make any sense too. The only application I see for it - is to perform transitions without exceptions.

And one more idea - Not to perform any transitions if the record is new:
@user = User.new
@user.new_record? # => true
@user.state # => "sleeping"
@user.run! # => TransitionException. "Record must be persisted before performing any transitions."
@user.run # => false. Record is not persisted.

That way it is extremely strict and hence - straightforward. Initial state cannot be skipped that way and must be persisted to the Database.

unabl4 commented Jan 18, 2013

I just had an idea, to use only the event method (bang or ordinary method) calls: run!, sleep!, etc just to switch from state to state. And to restrict direct assignment.

By the way. Using NON-bang methods doesn't make any sense too. The only application I see for it - is to perform transitions without exceptions.

And one more idea - Not to perform any transitions if the record is new:
@user = User.new
@user.new_record? # => true
@user.state # => "sleeping"
@user.run! # => TransitionException. "Record must be persisted before performing any transitions."
@user.run # => false. Record is not persisted.

That way it is extremely strict and hence - straightforward. Initial state cannot be skipped that way and must be persisted to the Database.

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto Feb 22, 2013

Member

So, you would argue in favor of suggestion 2 then? Obviously nobody else has a take in this issue. Thus we are free to decide ;)

Member

alto commented Feb 22, 2013

So, you would argue in favor of suggestion 2 then? Obviously nobody else has a take in this issue. Thus we are free to decide ;)

@johnnyshields

This comment has been minimized.

Show comment
Hide comment
@johnnyshields

johnnyshields Aug 25, 2013

Contributor

I think we should make this configurable. See #87 for "Slop mode" which would allow direct assignment and pick the first event which matches.

Contributor

johnnyshields commented Aug 25, 2013

I think we should make this configurable. See #87 for "Slop mode" which would allow direct assignment and pick the first event which matches.

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto Dec 16, 2013

Member

Sorry for getting back to this issue so late. Interestingly, after reading it again now, I recognised that I got it wrong right from the start. Only thing you suggested is this Direct assignment is not allowed thing, which I fully support. Will do this for AASM version 4. 👍

Member

alto commented Dec 16, 2013

Sorry for getting back to this issue so late. Interestingly, after reading it again now, I recognised that I got it wrong right from the start. Only thing you suggested is this Direct assignment is not allowed thing, which I fully support. Will do this for AASM version 4. 👍

@unabl4

This comment has been minimized.

Show comment
Hide comment
@unabl4

unabl4 Dec 19, 2013

Nice to hear :) Good luck. Looking forward to get a new version :)

unabl4 commented Dec 19, 2013

Nice to hear :) Good luck. Looking forward to get a new version :)

@jspooner

This comment has been minimized.

Show comment
Hide comment
@jspooner

jspooner Jan 6, 2014

This sounds like the issue I described here
#95 (comment)

I noticed a few specs in my project using update_attribute which would negate any event transition. I think this in combination with pull request #95 would help ensure models are always in valid states.

jspooner commented Jan 6, 2014

This sounds like the issue I described here
#95 (comment)

I noticed a few specs in my project using update_attribute which would negate any event transition. I think this in combination with pull request #95 would help ensure models are always in valid states.

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto Jan 11, 2014

Member

Working on this now. I recently saw somebody using direct assignment in order to let users interactively select the to-be-state. This is ambiguous and confusing (from a technical point of view), but still developers may choose to do so. I'm now thinking about making it configurable, and allowing direct-assignment by default.

Member

alto commented Jan 11, 2014

Working on this now. I recently saw somebody using direct assignment in order to let users interactively select the to-be-state. This is ambiguous and confusing (from a technical point of view), but still developers may choose to do so. I'm now thinking about making it configurable, and allowing direct-assignment by default.

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto Jan 11, 2014

Member

@unabl4 Are you using ActiveRecord with this?

Member

alto commented Jan 11, 2014

@unabl4 Are you using ActiveRecord with this?

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto Mar 17, 2014

Member

I assume you do.

Member

alto commented Mar 17, 2014

I assume you do.

@alto

This comment has been minimized.

Show comment
Hide comment
@alto

alto May 18, 2014

Member

Take a look at the aasm4 feature branch. :)

Member

alto commented May 18, 2014

Take a look at the aasm4 feature branch. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment