Skip to content
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

NULL aasm_state fix (validate => false) #9

Closed
wants to merge 4 commits into from
Closed

NULL aasm_state fix (validate => false) #9

wants to merge 4 commits into from

Conversation

unabl4
Copy link

@unabl4 unabl4 commented Jan 8, 2013

If you manage to make a new Model instance, e.g User.new, then persisting it with :validate => false option, e.g User.save(:validate => false) would cause it to store "NULL" state, which is not a good thing to come up with.

@ghost ghost assigned alto Jan 9, 2013
@alto
Copy link
Owner

alto commented Jan 9, 2013

Thanks for the pull request. But do you mind adding a test?

@unabl4
Copy link
Author

unabl4 commented Jan 9, 2013

:) I would be glad to. But, I'm pretty new to Ruby on Rails tests. I've tested it manually and it seemed to be working propertly.

By the way, is it hard to set the aasm_state attribute from the original model on instantiation ? e.g:
u = User.new
u.state # "default" (initial) instead. NOT nil

@alto
Copy link
Owner

alto commented Jan 12, 2013

Well, this is what the default (or initial) state is for. It makes sure the state is defined. And nil is not assumed to be a state. There is another issue asking for providing an initial state together with object initialization, like this:

u = User.new(aasm_state: 'not-default')
u.state
# => 'not-default'

But this is still on my list.

@alto
Copy link
Owner

alto commented Jan 12, 2013

Regarding your pull request, I'm not really happy to run aasm_ensure_initial_state twice now if you save the record with validation (which I think is the usual way to do). And dropping the lines with before_validation... is not a good thing either, because the validation may depend on having the initialized state. But I see your point. Maybe I can come up with another idea to solve this.

@unabl4
Copy link
Author

unabl4 commented Jan 13, 2013

Yeah. I'm also searching for a better solution here! But still, thank you for paying attention to the problem :)

@unabl4
Copy link
Author

unabl4 commented Jan 13, 2013

OK. How about this one ?

@alto
Copy link
Owner

alto commented Jan 17, 2013

That looks definitely better. But I would like to add some tests before merging it in. But won't find time for it for the next couple of weeks. Maybe you want to give it a try ;)

By the way, this is the last open issue for this repo, and it will be the last one. We changed the repo from aasm/aasm to alto/aasm, so that we could transfer the original repo rubyist/aasm over to aasm/aasm. Future issues will be put the again :-)

@unabl4
Copy link
Author

unabl4 commented Jan 17, 2013

Well, according to the Travis-CI system, my modifications didn't pass some of the tests there is. :( I would be very grateful to you, if you'd had these new tests written for me :)

Also, I had another proposal for the gem, which I have already implemented in my copy of the gem (it is NOT in repository) - To restrict direct aasm_column attribute assignment (setter that is), because it barely makes any sense, if we have events to transition from one state to another.
So, basically, it would look something like this:

@user = User.new
@user.state # sleeping
@user.state = :run # Some TransitionException exception thrown
@user.sleeping? # true. No state change
@user.run!
@user.running? # true. New state is active

I think it makes more sense to do it that way

@alto
Copy link
Owner

alto commented Jan 17, 2013

Indeed, the Travis build is broken. I will fix that when I'm back from my trip in 4 weeks.

Do you mind open another issue (on aasm/aasm) with your last proposal? That's an interesting topic, but not easy to change. Maybe others want to participate in that discussion as well.

@unabl4
Copy link
Author

unabl4 commented Jan 17, 2013

OK. Great. I've posted a new issue - aasm#53 ;)

@alto alto closed this May 3, 2013
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.

None yet

2 participants