copasetickid / draftsman Goto Github PK

Looks like a small problem with checking for the JSON data type in PostgreSQL; causes draft_destroy to fail.

NoMethodError - undefined method `type' for nil:NilClass:
  <path>/draftsman/lib/draftsman/draft.rb:37:in `previous_changes_col_is_json?'
  <path>/draftsman/lib/draftsman/model.rb:243:in `block (2 levels) in draft_destroy'
  activerecord (4.2.0) lib/active_record/connection_adapters/abstract/database_statements.rb:211:in `transaction'
  activerecord (4.2.0) lib/active_record/transactions.rb:220:in `transaction'
  activerecord (4.2.0) lib/active_record/transactions.rb:277:in `transaction'
  <path>/draftsman/lib/draftsman/model.rb:231:in `block in draft_destroy'
  ...

I'll submit a pull request shortly that fixes this.

Hey Chris,

This is more question/discussion than issue, in case you want to tag as such.

In our application, we're using the acts_as_list gem to organize a few sortable objects. If you're not familiar, acts_as_list essentially handles ordering items based on scope.

acts_as_list writes directly to the given model's table when updating the position column, which (in our case) causes it to bypass draftsman.

I was curious if you had any experience with acts_as_list, and if you had any clever integration methods. I understand the burden isn't really on draftsman -- acts_as_list would probably have to change to allow simple integration. I still thought I'd ask, just in case.

As a note, we may be moving away from acts_as_list, since it's causing a number of other little problems for us. If so, this is totally a non-issue.

Thanks!

Using https://github.com/RubyMoney/money-rails

This line make it crash when reify is called.

require self.item_type.underscore

https://github.com/RubyMoney/money-rails/blob/master/lib/money-rails/active_record/monetizable.rb#L250

raise ArgumentError, "#{self} already has a monetized attribute called '#{name}'"

Sometimes we may want to setup an aliased association with a scope, while including the original association as well.

I recently ran across the need to do something like this:

has_drafts

has_many :widgets
has_many :live_widgets, -> { live(:widgets) }, class_name: 'Widget'

The problem is that Draftsman will inspect both of those associations when querying for reversion and publication dependencies, which is unnecessary in many cases.

It makes sense to allow the option to only have Draftsman query one (or even neither) of those associations when identifying publication and reversion dependencies.

The API could possibly look like this:

has_drafts only_dependencies: [:widgets]

Or:

has_drafts without_dependencies: [:live_widgets]

Hey @chrisdpeters, I'm not sure how to fix this one, and thought you could help.

My application includes the draftsman gem, and I'm running Bundler version 1.9.9. When I run bundle exec rails c in my application, I get the following error:

Bundler is using a binstub that was created for a different gem.
This is deprecated, in future versions you may need to `bundle binstub railties` to work around a system/bundle conflict.

However, this shows up thousands of times, until I eventually get a "stack too deep" error, and the rails console never launches:

<path to rbenv>/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/bundler-1.9.9/lib/bundler/spec_set.rb:111:in `each': stack level too deep (SystemStackError)
    from <path to rbenv>/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/bundler-1.9.9/lib/bundler/spec_set.rb:111:in `find'
    from <path to rbenv>/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/bundler-1.9.9/lib/bundler/spec_set.rb:111:in `sorted'
    from <path to rbenv>/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/bundler-1.9.9/lib/bundler/rubygems_integration.rb:315:in `find'
    from <path to rbenv>/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/bundler-1.9.9/lib/bundler/rubygems_integration.rb:315:in `block in replace_bin_path'
    from <local path to draftsman>/draftsman/bin/rails:16:in `<top (required)>'
    from <local path to draftsman>/draftsman/bin/rails:16:in `load'
    from <local path to draftsman>/draftsman/bin/rails:16:in `<top (required)>'
    from <local path to draftsman>/draftsman/bin/rails:16:in `load'
     ... 10065 levels...
    from <local path to draftsman>/draftsman/bin/rails:16:in `load'
    from <local path to draftsman>/draftsman/bin/rails:16:in `<top (required)>'
    from <path to rbenv>/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/bin/rails:23:in `load'
    from <path to rbenv>/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/bin/rails:23:in `<main>'

I can run rails c locally just fine, but our staging and production environments require calling bundle exec, so they fail when trying to run. I'm a little out of my depth here, and not quite sure how these binstubs are used. This may fail for you too, on Bundler 1.9.9. Not sure if something changed in that release.

Thanks!

• Integrate with appraisal or gemika gem
• Add simple Docker + RVM config for testing different Ruby versions and DBs locally

This is a great gem! Even in the experimental state, it works incredibly well.

I found one issue when storing an attribute coded in JSON. When I reify from a draft, the attribute is returned as an encoded String rather than the expected JSON object.

As a brief example, assume I have a model with a settings attribute that looks something like this:

class Widget < ActiveRecord::Base
  store :settings, coder: JSON
end

In the database, the settings column is simply a text type field.

Assume I have a widget object and set its settings attribute to the following JSON object:
{"key1":"val1", "key2":"val2"}
If I run a draft_update, this will be saved into the drafts table correctly. Publishing also works as expected. However, reifying seems to return this object as a String rather than hash (or JSON object). widget.draft.reify.settings would return the following:
"{\"key1\":\"val1\", \"key2\":\"val2\"}"

I haven't yet found a workaround, but I'm still looking. If there's something simple you might recommend, let me know.

Thanks for all the development work you've put into draftsman!

This configuration doesn't seem to be picked up by the model:

Draftsman.draft_class_name = 'Draft'

It was working earlier running from my Mac on Rails 4.0. (I'm now running the app in Ubuntu, Rails 4.2)

In the meantime, I have to configure has_drafts like so:

has_drafts class_name: 'Draft'

Not a big issue, but there's a saving inconsistency when a model has a draft vs. when it does not.

When a model doesn't have a draft and both skipped and non-skipped attributes are updated, the skipped attributes are updated on the model, while the non-skipped attributes are updated in a new "update" draft.

When a model has a draft and both skipped and non-skipped attributes are updated, the skipped attributes are not updated on the model, and the non-skipped attributes are updated in a new "update" draft.

I'd assume they're both supposed to do the same thing -- either the skipped attributes are always updated or never updated. It doesn't really matter to me which way it goes, but it seems like a consistent update or non-update would be best in these cases.

I will write a test case and patch for this under the assumption that models should always be updated, but if that's the wrong assumption, we can flip it around.

RubyGems.org doesn't report a license for your gem. This is because it is not specified in the gemspec of your last release.

via e.g.

  spec.license = 'MIT'
  # or
  spec.licenses = ['MIT', 'GPL-2']

Including a license in your gemspec is an easy way for rubygems.org and other tools to check how your gem is licensed. As you can imagine, scanning your repository for a LICENSE file or parsing the README, and then attempting to identify the license or licenses is much more difficult and more error prone. So, even for projects that already specify a license, including a license in your gemspec is a good practice. See, for example, how rubygems.org uses the gemspec to display the rails gem license.

There is even a License Finder gem to help companies/individuals ensure all gems they use meet their licensing needs. This tool depends on license information being available in the gemspec. This is an important enough issue that even Bundler now generates gems with a default 'MIT' license.

I hope you'll consider specifying a license in your gemspec. If not, please just close the issue with a nice message. In either case, I'll follow up. Thanks for your time!

Appendix:

If you need help choosing a license (sorry, I haven't checked your readme or looked for a license file), GitHub has created a license picker tool. Code without a license specified defaults to 'All rights reserved'-- denying others all rights to use of the code.
Here's a list of the license names I've found and their frequencies

p.s. In case you're wondering how I found you and why I made this issue, it's because I'm collecting stats on gems (I was originally looking for download data) and decided to collect license metadata,too, and make issues for gemspecs not specifying a license as a public service :). See the previous link or my blog post about this project for more information.

Guys,

I rely heavly on https://github.com/mbleigh/acts-as-taggable-on . There was no way I could make it work.

Issues:

• When an acts-as-taggable-on tag is used outside "only" it does not bypasses it
• When an acts-as-taggable-on tag is used inside "only" it does not publish tags correctly

My User model is a Devise model and I'm using the trackable functionality. If I attempt to publish a change to the User model with object_changes unrelated to devise, I receive this error:

It seems related to this and this.

I know where the problem is happening, I just don't know enough about the issue I'm running into nor do I feel proficient in Ruby enough yet to know how I can help get it fixed or if it can be. The problem is happening during the reify call:

def reify
    without_identity_map do
      if !self.previous_draft.nil?
        reify_previous_draft.reify
      elsif !self.object.nil?

        [...]

        # The problem is happening in this loop when the key is equal to current_sign_in_ip or last_sign_in_ip
        attrs.each do |key, value|
          # Skip counter_cache columns
          if model.respond_to?("#{key}=") && !key.end_with?('_count')
            model.send "#{key}=", value
          elsif !key.end_with?('_count')
            logger.warn "Attribute #{key} does not exist on #{item_type} (Draft ID: #{id})."
          end
        end

        model.send "#{model.class.draft_association_name}=", self
        model
      end
    end
  end

I would love to help fix this if I better understood why setting these attributes was throwing this exception.

Hi, I'm the maintainer of PaperTrail. We just had an issue filed against the PaperTrail repo, paper-trail-gem/paper_trail#477, which was being caused by this gem.

I see that your gem here was cloned off of the PaperTrail source at one point. I wanted to advise you to modify your Sinatra extension so that it is not namespaced under the Sinatra module, as this can cause conflicts when other libraries look to see if Sinatra is defined. For more details see paper-trail-gem/paper_trail#277.

I would recommend simply adjusting your Sinatra module so that it is namespaced under the Draftsman module. You can mimick what we are doing on PaperTrail if that helps you.

1. Get rid of its.
2. Use expect instead of shoulda matchers:

it 'adds a user' do
  expect(user.count).to eql 3
end

1. Hardcode version numbers into development dependencies. Right now when you run Bundler, it installs version of RSpec and other gems that are incompatible with the code I wrote.

As @andymcintosh pointed out in #59, there may be an issue with saving a draft on a resource that already has a draft when there is no object_changes column in the drafts table.

Using Draftsman on a Rails 4.2 project, which adds foreign keys via the DB migrations.

So far, I've uncovered that calling draft_update to return a record back to its pre-draft state causes a PG::ForeignKeyViolation with Postgres.

I really need to get this repo running with Travis. (See #15.)

The purpose of this setting will be to not include draft_id in Draft#object and Draft#object_changes. It will be set to ignore by default.

When changed from the default, extra unneeded data is stored along with the draft, along with the potential to have unexpected results during object reification.

I'll need to experiment with this in the previous_draft attribute to make sure that omission of this data doesn't cause problems when reifying a trashed draft.

I have a Slide class which optionally belongs to an instance of a "slideable" class:

belongs_to :slideable, polymorphic: true

Line 79 of draft.rb assumes that calling my_item.slideable_type will always return a string that can be constantized to find the associated class.

my_item.send(association.foreign_key.sub('_id', '_type')).constantize

However, in my case, because I don't have slideable_type defined until I actually have a slideable associated which results in this error:

NoMethodError: undefined method `constantize' for nil:NilClass

I think I can work around this by having a default value for slideable_type set in Postgres but it seems like it might be fairly easy to add some logic to check if the polymorphic type is defined/valid when trying to determine the association class.

It would be nice if save_draft could add to errors
or have an optional mode where it could raise an exception in development mode so I knew why something wasn't working :-(

🌴 Aloha! 🌴

thanks very much for this gem! I'd been working on a very lo-fi version of this functionality, then said to heck with it and decided to check out your gem. I work on a lot of projects where publishing workflows are A Thing ™️

So, I started to fire things up, got this error, and wanted to pass on a quick fix with PR #38

I hope to contribute more in the future, thanks again.

The documentation states that publish! is available on a model instance that has_drafts, but the method is not actually available.

The documentation should be updated, or if in error, the publish! method should be available on the model instance.

Example query:

@content_template = current_site.content_templates.live
                                                  .includes(:draft, blocks: :draft, displays: :draft)
                                                  .where(blocks: { trashed_at: nil }, displays: { trashed_at: nil })
                                                  .find(params[:id])

Throws such an exception (in PostgreSQL):

PG::AmbiguousColumn: ERROR: column reference "trashed_at" is ambiguous LINE 1: ...id" WHERE "content_templates"."site_id" = $1 AND (trashed_at... ^ : SELECT DISTINCT "content_templates"."id" FROM "content_templates" LEFT OUTER JOIN "drafts" ON "drafts"."id" = "content_templates"."draft_id" LEFT OUTER JOIN "blocks" ON "blocks"."content_template_id" = "content_templates"."id" LEFT OUTER JOIN "drafts" "drafts_blocks" ON "drafts_blocks"."id" = "blocks"."draft_id" LEFT OUTER JOIN "displays" ON "displays"."content_template_id" = "content_templates"."id" LEFT OUTER JOIN "drafts" "drafts_displays" ON "drafts_displays"."id" = "displays"."draft_id" WHERE "content_templates"."site_id" = $1 AND (trashed_at IS NULL) AND "blocks"."trashed_at" IS NULL AND "displays"."trashed_at" IS NULL AND "content_templates"."id" = $2 LIMIT 1

Or at least publish! should receive arguments that will be passed to save.

I have to validate that the record has to meet certain criteria when it is published but not when it is drafted.

when i load rails console with the latest rspec-rails (3.5.2) together with draftsman i get this warning.

Loading development environment (Rails 4.2.7.1)
irb: warn: can't alias context from irb_context.
2.2.5 :001 >

While it's a cute name from the paper_trail project, it's also super annoying. :)

Given:

class Widget < ActiveRecord::Base
  accept_nested_attributes_for :cars, allow_destroy: true
  has_drafts ignore: [:slug]
end

class WidgetsController < InheritedResources::Base
  def update
    @widget.attributes = widget_params # Update nested attributes already here
    if @widget.draft_update
     # ok
    else
      update! # Inherited resources update without draft, raising exception if destroyed or create duplicates because they were created in above code
    end
  end
end

the problem - nested object are created or destroyed twice(raising not found on second destroy).

Does anyone faced with similar problem ?

This is more in line with the naming that Rails chooses for methods like marked_for_destruction?.

Add deprecation notice when draft_destroy is called.

Per discussion in #34, we need to have some tests to make sure that Draftsman works with models containing enums moving forward.

If anyone would like to contribute, let us know. Otherwise, I'll get to this soon.

If a model's has_one association is nil, draft_destroy will throw an exception.

For example, if I have a Parent which has_one :child, but the child doesn't exist yet (so the association is nil), a draft_destroy will throw a NoMethodError.

NoMethodError - undefined method `draft?' for nil:NilClass:
  <path>/draftsman/lib/draftsman/model.rb:282:in `block (4 levels) in draft_destroy'
  <path>/draftsman/lib/draftsman/model.rb:279:in `block (3 levels) in draft_destroy'
  <path>/draftsman/lib/draftsman/model.rb:270:in `block (2 levels) in draft_destroy'

This will occur in traditional and polymorphic has_one associations.

Edit: By simply adding a has_one association to the Parent class in the current dummy specs, you'll find that 29 specs fail.

There are scenarios where it makes sense to stash drafted data and require reification to get it back. And #51 could potentially make it more performant in most situations.

There are other scenarios where you may be creating a service that is only concerned with drafted data, so reification is not required or necessary. I'm finding myself in that situation with my current project, and I'd imagine that others would too.

This could be done via a setting or by simply removing the object column from the database. I am open to comments about which would be more appropriate. I'm leaning toward making it an explicit setting so that the intention is documented somewhere in the application's source code.

• Add Draftsman.stash_drafted_changes config option
  • Add to generator
• Apply new logic to Model#save_draft method
• Apply new logic to Draft#reify method
• Apply new logic to Draft#publish! method
• Apply new logic to Draft#revert! method
• Apply new logic to Model#draft_destruction - delete record entirely and stash it in Draft
• Update documentation

The drafted state is already stored in the item's record anyway. No need to do the extra work, so Draftsman::Draft#reify can just return the item.

Hey Chris, this is a weird one, and might be worth a conversation.

Let's say you have Book and Shelf models. A Shelf has_many books, and a Book can only be on one Shelf at any given time.

Further, assume some Book is on Shelf 1, and you decide you want to move it to Shelf 2, and this is done via draftsman. Your Book will have an association of Shelf 1 (its old location), and the Book's draft will have an association of Shelf 2 (its new location).

If you draft_destroy Shelf 1, the Book should still exist because its draft is on Shelf 2. However, currently draft_destroy handles destroying all dependencies. The issue is that it uses all non-drafted dependencies, regardless of whether the association has changed.

In the case above, this would mean that the Book would be draft_destroyed when Shelf 1 was draft destroyed, even though the Book('s draft) is on Shelf 2.

I tried to simplify the example above, but this was somewhat confusing to write up. If I can clarify anything, please let me know. If you'd like to communicate another way (email/skype/phone), we would be more than happy to talk about it, since there doesn't seem to be an obvious solution.

Thanks Chris!

My situation is that I have a tree structure, and when I trash one node, I want to stitch that node's children to the node's parent node.

Currently, when I call draft_destruction, all of the children nodes are also trashed.

I have a model defined like this:

class Question < ActiveRecord::Base
    ...
    enum question_response_type: { radio: 0, text: 1 }
    ...
end

When I try to call the reify method on a question draft, ActiveRecord throws an error:

ArgumentError: You tried to define an enum named "question_response_type" on the model "Question", but this will generate a instance method "question_response_type=", which is already defined by another enum.

Is there any way around this?

I thought I could use Draftsman to save and update drafts of objects without also saving to the original object, but it looks like the draft_update method updates and saves both the original object and the draft.

Right now I'm saving an object (a post) as a draft until published, and I'm publishing it through a custom publish action in my controller. After publishing I'd like to be able to go back and create/update a draft of that published post without also updating the published post. I can't seem to find a method in the documentation that does this.

Is there a method that updates just the draft so that a published object isn't immediately updated when its draft is updated?

Need to review logic for draft_reversion_dependencies as well.

Identified the fix and working on writing tests that capture the bug before applying the fix.

Hey Chris,

I forked draftsman to add a small piece of functionality, and wanted to send you a pull request. However, I'm having trouble getting the specs to run, and didn't want to send a pull request without running (and adding) tests.

When I run RAILS_ENV=test rake db:migrate (or other db and spec tasks), I get:

rake aborted!
LoadError: cannot load such file -- rack/test
/<my path>/draftsman/spec/dummy/config/application.rb:4:in `<top (required)>'
/<my path>/draftsman/spec/dummy/Rakefile:5:in `<top (required)>'
LoadError: cannot load such file -- active_record/railtie
/<my path>/draftsman/spec/dummy/config/application.rb:4:in `<top (required)>'
/<my path>/draftsman/spec/dummy/Rakefile:5:in `<top (required)>'

I was following your listed steps in the "Contributing" section of the docs. Sorry if I'm missing something obvious here!

Thanks again!
Nick

Like AR's #save, it should intelligently call #draft_creation or #draft_update based on the state of the object being drafted.

Idea originally from #46.

Hey Chris,

It's been a (very) long time, but I'm finally looping back around to putting drafts into our application. Today I ran across an issue, and I have a 100% reproducible case.

I created a simple app with the Postgres JSON data type (and used the install switch to enable it for drafts). The app has a single table and model, Widget, and I added drafts to this table.

If I create a draft Widget, then later draft_destroy this Widget (without publishing in between), I get a JSON::ParserError.

For example, in the console, I can do the following to reproduce:

w = Widget.new(some_attr: "hello")
w.draft_creation

At this point, I can see the Widget and draft, and all looks good. Running w.draft_destroy will result in:

JSON::ParserError: 795: unexpected token at '---
id: 1
item_type: Widget
item_id: 1
event: create
whodunnit:
object:
  id: 1
  some_attr: hi
  created_at: '2015-03-23T22:42:15.628Z'
  updated_at: '2015-03-23T22:42:15.628Z'
  draft_id:
  published_at:
  trashed_at:
previous_draft:
created_at: 2015-03-23 22:42:15.651663000 Z
updated_at: 2015-03-23 22:42:15.651663000 Z
'

I'm not sure if we're supposed to handle draft deletion differently for non-published content. If so, I can just change what I'm doing.

Let me know if you need more info!

Thanks!
Nick

Came across this issue when working with drafts that may have attachments on rails 3.2.8 (although suspect it affects newer versions).

Attachments are saved on save but doesn't look like draft_update triggers this method so images get marked to be saved but never moved since the save method doesn't get triggered.

While this is in an issue with paperclip, curious if there's a work around that you maybe able to recommend?

The first line of the migration file it generate when you run rails g draftsman:install looks like this:

class CreateDrafts < ActiveRecord::Migration

If you run rake db:migrate rails throws an error:

rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

Directly inheriting from ActiveRecord::Migration is not supported. Please specify the Rails release the migration was written for:

class CreateDrafts < ActiveRecord::Migration[4.2]

I think the error message says it all! The code outputted by the generator should not directly inherit from ActiveRecord::Migration.

I posed a question to @npafundi here in #47. I've since merged into master so I can work on some other features without an awful merge conflict.

Let's go ahead and have the discussion and track progress with callbacks in this issue.

Here is the question:

@npafundi I think that this is a good move for the API of this gem. It is a little confusing knowing when to call #draft_creation vs. #draft_update.

However, this could potentially affect how we do callbacks. If drafting behavior is moved into a single method, I'm not convinced that it would be clear to users of this gem when *_draft_creation vs. *_draft_update callbacks would be triggered. I think it's a little ambiguous: is before_draft_creation called when a draft record is being first created? Or is it when the item is created? See what I mean?

So I must ask, which of the callbacks are you using right now? Would you miss anything if I removed the ambiguous ones and created more general before_save_draft, after_save_draft, and around_save_draft callbacks?

I'm working with Rails 4.2 and I'm getting the following warning:

DEPRECATION WARNING: `serialized_attributes` is deprecated without replacement, and will
be removed in Rails 5.0.

This issue is similar to paper-trail-gem/paper_trail#416

I added the following patch (to suppress deprecation warnings) https://github.com/dpaluy/draftsman/blob/afce35b3985c79760176f31710c11a77b1201f0e/config/initializers/draftsman.rb

This demonstrates the bug:

class Skipper < ActiveRecord::Base
  has_drafts skip: [:skip_me]
end

skipper = Skipper.create!(:name => 'Bob', :skip_me => 'Skipped 1')

skipper.name = 'Sam'
skipper.skip_me = 'Skipped 2'
skipper.draft_update
skipper.reload

skipper.skip_me
=> 'Skipped 1' # Should be 'Skipped 2'

Also remove associated callbacks.