Parameterize taxon's permalink also on update #3090
Conversation
| permalink_tail = permalink.split('/').last if permalink.present? | ||
| permalink_tail ||= Spree::Config.taxon_url_parametizer_class.parameterize(name) | ||
| self.permalink_part = permalink_tail | ||
| permalink_tail = permalink.present? ? permalink.split('/').last : name |
There was a problem hiding this comment.
Do you think we can use permalink_part_changed? here to change the permalink_part only when it is changed?
There was a problem hiding this comment.
I'll change that, it's better yes.
There was a problem hiding this comment.
Actually, we cannot really use that condition because this method is called also for all children via:
after_update :update_child_permalinks, if: :saved_change_to_permalink?
...
def update_permalinks
set_permalink
# This will trigger update_child_permalinks if permalink has changed
save!
end
it breaks current test, I will leave it this way.
There was a problem hiding this comment.
Makes sense, can't see any clean solution here. I agree it's better to keep it as is.
| taxon.update_attributes(permalink: 'spécial&charactèrs') | ||
| expect(taxon.permalink).to eql "special-characters" | ||
| end | ||
| end |
There was a problem hiding this comment.
This change also has effects on permalinks of children. Can we test that it works with children as well?
There was a problem hiding this comment.
ok, I'll add this test.
There was a problem hiding this comment.
I think the test you added is testing that the permalink is correctly saved when it has a parent taxon. I mean to test when the taxon with special chars has children instead (like spécial&charactèrs/child for example) so we can see the parameterization reflected on those taxons as well. Anyway the test you added can stay, better to have that as well. 🙂
There was a problem hiding this comment.
I added 2 more tests
core/spec/models/spree/taxon_spec.rb
Outdated
| @@ -26,6 +26,14 @@ | |||
| expect(taxon.permalink).to eql "ruby-on-rails" | |||
| end | |||
|
|
|||
| context "updating a taxon permalink" do | |||
| it 'parameterize permalink correctly' do | |||
There was a problem hiding this comment.
it's parameterizes here, right?
| taxon.update_attributes(permalink: 'spécial&charactèrs') | ||
| expect(taxon.permalink).to eql "special-characters" | ||
| end | ||
| end |
There was a problem hiding this comment.
I think the test you added is testing that the permalink is correctly saved when it has a parent taxon. I mean to test when the taxon with special chars has children instead (like spécial&charactèrs/child for example) so we can see the parameterization reflected on those taxons as well. Anyway the test you added can stay, better to have that as well. 🙂
loicginoux
force-pushed
the
fix_taxon_permalink_edition
branch
from
a1627f6 to
584c3da
Compare
|
ok, all in one commit now. thx for the review ! |
kennyadsl
changed the title
There was a problem hiding this comment.
@loicginoux thanks for this PR! 🍰
I noticed that the commit includes the sentence correct grammar on test description twice, can please fix it?
Again, thanks for the changes 👍
There was a problem hiding this comment.
I think this works, please address the @spaghetticode comment if you have time.
add test for parameterize taxon permalink when taxon parent solidusio#3086 correct grammar on test description test parameterization on children taxon permalink solidusio#3086
loicginoux
force-pushed
the
fix_taxon_permalink_edition
branch
from
584c3da to
01509d4
Compare
|
Ok I changed commit description and removed one |
|
Thanks @loicginoux! |
Description
fix bug #3086
the fix allows to use the
Spree::Config.taxon_url_parametizer_classon taxon permalink update, and not only on the name. We insure permalink is always parameterized on save.