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.

Sign up for GitHub

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

Jump to bottom

1.22 breaking change #352

Closed
Gasparila opened this issue May 10, 2019 · 2 comments · Fixed by flexport/algoliasearch-rails#2 or #354
Closed

1.22 breaking change #352

Gasparila opened this issue May 10, 2019 · 2 comments · Fixed by flexport/algoliasearch-rails#2 or #354

Comments

@Gasparila
Copy link
Contributor

Gasparila commented May 10, 2019 •
edited
Loading

  • Rails version: 5.1.6
  • Algolia Rails integration version: 1.22

Description

#338 introduced a few
breaking changes (including 1 fixed in #350).

https://github.com/algolia/algoliasearch-rails/pull/338/files#diff-dfaec67e907148ea8211e17340f0f30cR749 changed the behavior
for when <attribute>_changed? doesn't exist. Previously it would
be conservative and always auto-index if there are custom attributes.
Now it will assume no changes are necessary

Steps To Reproduce

class AnyModel < ActiveRecord::Base
  include AlgoliaSearch
  algoliasearch do
    attribute(:not_a_column) { some_column + 1 }
  end
end

instance_of_model.update!(some_column: 3) # will not trigger algolia update
Gasparila added a commit to flexport/algoliasearch-rails that referenced this issue May 11, 2019
@Gasparila
Fix algolia#352 and algolia#353
c762449 
Fix breaking change introduced in algolia#352
According to the [documentation](https://github.com/algolia/algoliasearch-rails/blob/master/lib/README.md#630),
using custom attributes without a custom `_changed?` method will always
cause changes to push to the API. This behavior was lost in algolia#338

Fix unexpected behavior during transactions algolia#353
`@algolia_must_reindex` is reset in every validate call. This means that a record
that is updated multiple times in a transaction will only be indexed if the
final update marks as must reindex. Update the code so that once
`@algolia_must_reindex` is marked as `true`, it remains true until
explicitly unset in `algolia_perform_index_tasks`
@Gasparila Gasparila mentioned this issue May 11, 2019
Merged
@Spone
Copy link
Contributor

Spone commented May 11, 2019

We've experienced the same bug and it was hard to debug... Please Algolia be careful with regressions 🙏

Gasparila added a commit to flexport/algoliasearch-rails that referenced this issue May 11, 2019
@Gasparila
Fix algolia#352 and algolia#353
1b1a203 
Fix breaking change introduced in algolia#352
According to the [documentation](https://github.com/algolia/algoliasearch-rails/blob/master/lib/README.md#630),
using custom attributes without a custom `_changed?` method will always
cause changes to push to the API. This behavior was lost in algolia#338

Fix unexpected behavior during transactions algolia#353
`@algolia_must_reindex` is reset in every validate call. This means that a record
that is updated multiple times in a transaction will only be indexed if the
final update marks as must reindex. Update the code so that once
`@algolia_must_reindex` is marked as `true`, it remains true until
explicitly unset in `algolia_perform_index_tasks`
@Gasparila Gasparila mentioned this issue May 11, 2019
Merged
Gasparila added a commit to flexport/algoliasearch-rails that referenced this issue May 13, 2019
@Gasparila
Merge pull request #2 from flexport/kevin/fix_transaction_issues
68d3ba8 
Fix algolia#352 and algolia#353
@Gasparila
Copy link
Contributor Author

Reopening because this was fixed in our fork (which you are welcome to use @Spone) but has not yet been merged into this repo. PR here: #354

@Gasparila Gasparila reopened this May 13, 2019
julienbourdeau added a commit that referenced this issue Jun 24, 2019
@julienbourdeau
Merge pull request #354 from flexport/kevin/fix_transaction_issues
3ba8f51 
Fix #352 and #353
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #352 and #353 flexport/algoliasearch-rails
Fix #352 and #353 flexport/algoliasearch-rails
2 participants
@Spone @Gasparila