mrbrdo / spree_mobility

BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

Can't create Taxonomy with Spree 4.4 #6

Closed Kulgar closed 2 years ago

Kulgar commented 2 years ago

Hello again :-)

I faced a new issue today: I'm no longer able to create new taxonomies.

After some digging, I found that the culprit is that line :

validates :name, presence: true

In the taxon_decorator.rb file.

Indeed, when we create a new Taxonomy, it automatically creates a new taxon :

# from spree core

    after_create :set_root
    after_save :set_root_taxon_name

    private

    def set_root
      self.root ||= Taxon.create!(taxonomy_id: id, name: name)
    end

    def set_root_taxon_name
      return unless saved_change_to_name?
      return if name.to_s == root.name.to_s

      root.update(name: name)
    end

In Spree 4.4, the Taxon name validation is this:

    validates :name, presence: true, uniqueness: { scope: [:parent_id, :taxonomy_id], allow_blank: true, case_sensitive: false }

I don't really have time to investigate more... but, right now, deactivating the validates in the decorator fixes my problem. Do you have any idea on a possible fix?

Thx!

mrbrdo commented 2 years ago

I can immediately confirm this as I had the same problem on a new Spree (5.alpha) project. I came to the exact same code in spree core as you did, and after some time I was not able to exactly figure out what is going on. I think the issue is related to mobility itself (not spree_mobility). The interaction between those 2 after_create/save hooks and mobility makes it so the name is not correctly read from translations, but instead from the main table where it is nil (which is normally fine). As far as why it happens I'm not sure at this point, but I have a feeling it's a bug/issue in mobility gem. Besides that set_root_taxon_name is also slightly bugged in that it doesn't check if root is present.

The presence validator is not an "issue" by itself, actually it is a feature compared to spree_globalize, which didn't have validations on translations models - which means you basically can have invalid data in the DB which can cause all sort of issues. Ideally the uniqueness validation should also be added to translations yeah, iirc it was a little too complicated at this point though (translations table doesn't directly have parent_id/taxonomy_id so it'd have to go through a join - custom validator).

mrbrdo commented 2 years ago

@Kulgar okay so I figured out what's going on. Another after_save hook (sync_taxonomy_name) was added in 4.4.x to Taxon, which triggers after Taxonomy is created and before Taxonomy translations are created in DB. Although possibly mobility can be improved for such situation to not be a problem, avoiding this hook on create (when it is not necessary anyway) fixes it. I fixed this in https://github.com/mrbrdo/spree/commit/45e167902d81a848cd5b4a3b8173766811933667 on my 4_4_fixes branch of spree (which is actually 5.alpha).

Kulgar commented 2 years ago

@mrbrdo : thanks for the investigation!

Weird, as I did solve / fix the bug by just commenting this line in spree mobility decorator:

https://github.com/mrbrdo/spree_mobility/blob/0ec8da801d3307ccec57c65c8cf698af36f87eb1/lib/spree_mobility/core_ext/spree/taxon_decorator.rb#L44

I didn't have to patch Spree 4.4 in any way to make it work...

mrbrdo commented 2 years ago

Yeah, but now you don't have validation on the name. Which means you can save a Taxon with an empty name, but that shouldn't be allowed.

Kulgar commented 2 years ago

ok, understood, but patching Spree isn't an option for me right now :-)

mrbrdo commented 1 year ago

@Kulgar spree just merged the fix into main branch: https://github.com/spree/spree/pull/11705#event-7691049345

Kulgar commented 1 year ago

nice, thanks @mrbrdo