geekq / workflow

Ruby finite-state-machine-inspired API for modeling workflow
MIT License
1.75k stars 207 forks source link

Negative class level and instance level scopes #112

Closed voltechs closed 5 years ago

voltechs commented 10 years ago

Go ahead and ignore e9c1c61. (incase it wasn't obvious)

geekq commented 10 years ago

There is a 'not' operator ! in Ruby. ;-) So instead of adding additional methods like article.not_awaiting_review? I can use !article.awaiting_review?

"negative" scopes can be very helpful though. My question regarding implementation - is

where("#{table_name}.#{self.workflow_column.to_sym} != ?", state.to_s)

particularly != database specific or will it work with every database?

voltechs commented 10 years ago

Oh thats a good question! Let me play some more with it.

I'm aware of the ! operator in Ruby, but I wasn't aware it would translate into SQL. This is particularly useful when dealing with scopes because I don't think you can say (for example) !User.active and get all the Users that are not active. I'll try it out in rails c

Also, when you say "every database", is that truly comprehensive or do you mean all SQL databases (MSSQL, MySQL, PostgreSQL). As I was looking for a list of SQL databases I came across this http://en.wikipedia.org/wiki/SQL#Operators

I'd be happy to change != to <>

voltechs commented 10 years ago

@geekq How do you feel about this PR now?

Also, when do you think the next release of workflow.gem will be? I noticed some (strange?) new behavior running off master, where the current_state.events returns a hash of arrays. This seems different than the 1.1.0 gem.

[22] pry(main)> dr.current_state.events                                                                                                                                                                                                                                                                                                                                     
=> {:suspend=>
  [#<Workflow::Event:0x007f9be9090858
    @action=nil,
    @condition=nil,
    @meta={},
    @name=:suspend,
    @transitions_to=:suspended>],
 :resign=>
  [#<Workflow::Event:0x007f9be90907b8
    @action=nil,
    @condition=nil,
    @meta={},
    @name=:resign,
    @transitions_to=:resigned>],
 :vacation=>
  [#<Workflow::Event:0x007f9be9090678
    @action=nil,
    @condition=nil,
    @meta={},
    @name=:vacation,
    @transitions_to=:vacation>]}

Is this intentional? I couldn't locate the source of this. (I didn't git-bisect)

geekq commented 10 years ago

Thanks for spotting this behaviour change! There are extensive tests for usual state machine behaviour, but not so many for returning meta information like which states have got which events.

This was likely introduced by the integration of one of recent pull requests. https://github.com/geekq/workflow/commit/ff471575678bb4e9a7b76246c6ee0cecf8c8a2e7#diff-d41d8cd98f00b204e9800998ecf8427e https://github.com/geekq/workflow/pull/109

I found the changes in #109 in overall very good. Now is the question: try to restore the old behaviour keeping the new functionality or just increase the version number and document the changed behaviour for API returning meta information? I tend to the last solution.

geekq commented 10 years ago

Regarding the pull request itself:

(+) activerecord.rb: the without... scope is a single feature which makes sense for me. We can add that.

(-) README.markdown: just reformatted?

(?) what about compatibility of '<>' with different databases, did you test this at least on postgres, sqlite, mysql and one commercial database? Anyway instead of using strings for that, I would suggest using Arel for building sql with widest possible compatibility.

(?) test: I think testing for the presence of a method does not help much. I found testing with creating some records and checking if the without_ really delivers a desired subset of records is better. Please s.also https://github.com/geekq/workflow/blob/master/test/advanced_hooks_and_validation_test.rb#L68 how this can be done.

nickgrim commented 9 years ago

what about compatibility of '<>' with different databases

@voltechs: have you considered using:

eigenclass.send(:define_method, "without_#{state}_state") do
  where.not("#{table_name}.#{self.workflow_column.to_sym} = ?", state.to_s)
end

...and deferring database cross-compatibility to ActiveRecord?

jlovell commented 9 years ago

using arel for database independence:

eigenclass.send(:define_method, "without_#{state}_state") do
  where(arel_table[self.workflow_column.to_sym].not_eq(state.to_s))
end

@nickgrim where.not works the same but is only available in Rails 4.x