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

feat: add support for Newspack Guest Contributor in HPP blocks #1934

Merged
merged 5 commits into from
Nov 26, 2024
Merged

feat: add support for Newspack Guest Contributor in HPP blocks #1934

merged 5 commits into from
Nov 26, 2024

Conversation

leogermani
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Adds support for the Newspack's Guest Contributor role in the Homepage posts and Carousel blocks

How to test the changes in this Pull Request:

  1. Create a user with only the Guest Contributor role and assign a post to it
  2. Add a Home page posts block
  3. Search by that author name and confirm it finds the author and their posts
  4. Do the same for the Carousel block

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@leogermani leogermani self-assigned this Oct 30, 2024
@leogermani leogermani requested a review from a team as a code owner October 30, 2024 19:46
dkoo
dkoo requested changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

I'm not sure why it's happening, but when I have ElasticSearch enabled for HPP and Carousel blocks the posts aren't getting fetched for me on the front-end. It works as expected in the editor:

Screenshot 2024-11-04 at 2 37 00 PM

But the front-end shows 0 posts for both blocks. I confirmed that the query args are the same in both contexts, and I only have a single Guest Contributor user who's assigned to a single post (but these were created/assigned many weeks ago, so they should be indexed for ElasticSearch by now). Paging @miguelpeixe who worked on the ES integration in case it's obvious to you.

@leogermani
Copy link
Contributor Author

Looks like requests in the admin are not offloaded to ES.

I'm struggling to test this. @miguelpeixe could you have a look?

@leogermani
Copy link
Contributor Author

@dkoo To be fair, this PR does not touch the Query for posts. It only touches the query for authors in the Authors filter input...

@miguelpeixe
Copy link
Member

miguelpeixe commented Nov 19, 2024 •
edited
Loading

Paging @miguelpeixe who worked on the ES integration in case it's obvious to you.

That was an issue in the first implementation of the ES, which has been fixed since https://github.com/Automattic/newspack-manager/pull/275. Can you confirm you have the latest manager?

I may have misunderstood the issue. It works in the editor but on the front-end. If you have ES enabled for Newspack Blocks that might be a new uncovered incompatibility. We should flag that in Asana for further exploration.

@miguelpeixe
Copy link
Member

It only touches the query for authors in the Authors filter input...

The ES incompatibility might be in trunk, to any query filtering CAP authors.

@leogermani
Copy link
Contributor Author

It only touches the query for authors in the Authors filter input...

The ES incompatibility might be in trunk, to any query filtering CAP authors.

They are not CAP authors. It's just another user role. But they are regular wp users

@leogermani
Copy link
Contributor Author

I may have misunderstood the issue. It works in the editor but on the front-end.

afaik queries are not offloaded to ES in the admin, only in the front end

@leogermani
Copy link
Contributor Author

leogermani commented Nov 25, 2024 •
edited
Loading

@dkoo can you have a look again, without the ES integration? I've updated this branch with the latest trunk, there were some fixes there. Things work ok on my end.

I'll work on another PR to make sure that the queries that have the SQL statement filtered will not be offloaded to ES.

EDIT: actually it was a one liner so I added in 1d1b168 - depends on https://github.com/Automattic/newspack-manager/pull/301

@leogermani leogermani requested a review from dkoo November 25, 2024 14:50
dkoo
dkoo approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Working now with https://github.com/Automattic/newspack-manager/pull/301!

@dkoo
Copy link
Contributor

dkoo commented Nov 26, 2024

@leogermani looks like we need to account for the new query param in the unit tests

@leogermani
Copy link
Contributor Author

@leogermani looks like we need to account for the new query param in the unit tests

Fixed

@leogermani leogermani requested review from dkoo November 26, 2024 13:45
@leogermani leogermani merged commit c16849e into trunk Nov 26, 2024
7 checks passed
@leogermani leogermani deleted the feat/add-support-for-guest-contributor branch November 26, 2024 13:47
@github-actions GitHub Actions
Copy link

Hey @leogermani, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

matticbot pushed a commit that referenced this pull request Nov 29, 2024
@semantic-release-bot
chore(release): 4.5.0-alpha.1 [skip ci]
b0b3678 
# [4.5.0-alpha.1](v4.4.0...v4.5.0-alpha.1) (2024-11-29)

### Bug Fixes

* also search for coauthor posts by term slug ([#1954](#1954)) ([49357ff](49357ff))
* **ras-acc:** correct spacing issue around saved credit cards ([#1980](#1980)) ([52a5c57](52a5c57))
* **ras-acc:** fix display issues with Additional Fields ([#1979](#1979)) ([b9390ef](b9390ef))
* **ras-acc:** remove space caused by empty divs ([#1978](#1978)) ([8cb6ead](8cb6ead))

### Features

* add Bluesky support to the Author Profile, List blocks ([#1969](#1969)) ([d26a7e4](d26a7e4))
* add support for Newspack Guest Contributor in HPP blocks ([#1934](#1934)) ([c16849e](c16849e))
* merge RAS-ACC work into trunk ([#1977](#1977)) ([2eeaa89](2eeaa89))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.5.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Dec 9, 2024
@semantic-release-bot
chore(release): 4.5.0 [skip ci]
175c966 
# [4.5.0](v4.4.0...v4.5.0) (2024-12-09)

### Bug Fixes

* also search for coauthor posts by term slug ([#1954](#1954)) ([49357ff](49357ff))
* **modal-checkout:** allow all gateway assets ([#1988](#1988)) ([e371e30](e371e30))
* **modal-checkout:** handle paypal ([#1985](#1985)) ([9bb2b8c](9bb2b8c))
* **ras-acc:** correct spacing issue around saved credit cards ([#1980](#1980)) ([52a5c57](52a5c57))
* **ras-acc:** fix display issues with Additional Fields ([#1979](#1979)) ([b9390ef](b9390ef))
* **ras-acc:** remove space caused by empty divs ([#1978](#1978)) ([8cb6ead](8cb6ead))
* remove reCaptcha for WooCommere code from modal checkout ([#1984](#1984)) ([8e250eb](8e250eb))

### Features

* add Bluesky support to the Author Profile, List blocks ([#1969](#1969)) ([d26a7e4](d26a7e4))
* add CSS class to variation buttons for tracking ([#1989](#1989)) ([910e6b1](910e6b1))
* add support for Newspack Guest Contributor in HPP blocks ([#1934](#1934)) ([c16849e](c16849e))
* merge RAS-ACC work into trunk ([#1977](#1977)) ([2eeaa89](2eeaa89))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkoo dkoo Awaiting requested review from dkoo

Assignees

@leogermani leogermani

Labels
released on @alpha released [Status] Approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leogermani @miguelpeixe @dkoo @matticbot