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

Index comments on Solr #152

Merged
merged 2 commits into from
Mar 22, 2019
Merged

Index comments on Solr #152

merged 2 commits into from
Mar 22, 2019

Conversation

gforcada
Copy link
Member

@gforcada gforcada commented Mar 21, 2019 •
edited
Loading

As collective/collective.solr#102 finally got merged, it is time to use that on plone.app.discussion.

This minimal pull request is what is needed so that collective.solr (or other indexing tools) need to index comments on Solr.

Fixes (by ignoring) #77

@mister-roboto
Copy link

@gforcada thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@gforcada
Copy link
Member Author

@jenkins-plone-org please run jobs

@gforcada gforcada mentioned this pull request Mar 21, 2019
Closed
ale-rt
ale-rt approved these changes Mar 21, 2019
View reviewed changes
Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust you and I see the big plus in switching from catalog.reindexObject(obj) to obj.indexObject() to exploit the queue.
It is not evident for me why the method in the tool.py file were removed.
Anyway I would say let's merge this :)

@gforcada
Copy link
Member Author

@ale-rt as far as I could see the methods there on tool.py are only used by the deprecated #114, but you are right, I should have kept them, removing a deprecated tool has nothing to do with this.

I will update the pull request, thanks for the review!! 🙇‍♂️

gforcada added 2 commits March 21, 2019 08:57
@gforcada
index/reindex/unindex the object itself
f217a42 
Do not defer to p.a.discussion own tool.

That's needed so that collective.indexing can patch comments,
so in turn can be indexed by Solr.
@gforcada
Add news entry
8cb659c
@gforcada gforcada force-pushed the index-comments-on-solr branch from a5c0c38 to 8cb659c Compare March 21, 2019 07:57
@ale-rt
Copy link
Member

ale-rt commented Mar 21, 2019

Thanks for clarifying! It's even fine to remove them if they are deprecated :)

@gforcada
Copy link
Member Author

@jenkins-plone-org please run jobs

@gforcada
Copy link
Member Author

I was about to close this one, but given that it still helps on removing the last usage of the deprecated comment indexing tool, I will keep it.

Let's see if the unrelated failures are fixed on a second run.

@gforcada gforcada merged commit bc42fb2 into plone:master Mar 22, 2019
@gforcada gforcada deleted the index-comments-on-solr branch March 22, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ale-rt ale-rt ale-rt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gforcada @mister-roboto @ale-rt