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.

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

Closes #5885: Preload: Don't try to preload private pages #5920

Merged
merged 34 commits into from
Jun 27, 2023

Conversation

jeawhanlee
Copy link
Contributor

Description

Checks for posts with private status and exclude from preload.

Fixes #5885

Type of change

  • Enhancement (non-breaking change which improves an existing functionality)

Is the solution different from the one proposed during the grooming?

No

How Has This Been Tested?

Automated tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@jeawhanlee jeawhanlee self-assigned this May 12, 2023
@jeawhanlee jeawhanlee added module: preload type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels May 12, 2023
@jeawhanlee jeawhanlee requested a review from a team May 12, 2023 16:33
@jeawhanlee jeawhanlee marked this pull request as ready for review May 12, 2023 16:33
@Mai-Saad Mai-Saad self-requested a review May 15, 2023 16:01
@jeawhanlee jeawhanlee requested a review from engahmeds3ed May 16, 2023 13:41
@Mai-Saad
Copy link
Contributor

@jeawhanlee Thanks for the PR. Now adding new private post / editing content of existing private post won't add any entry in the cache table.

However, @piotrbak do you think we need to do change for any of the following?

  1. if we edited public cached post to private, we won't remove the entry from the cache table till manual cache clear or change settings. can we delete the URL in this case?
  2. if custom sitemap contains private URL, it will be added to the table then removed once processed.
  3. if we edited private post, home, author (prev/next if not private) will be preloaded

@piotrbak
Copy link
Contributor

@Mai-Saad 2 and 3 are okay.

For the first one, it's acceptable, but it would be better if we could remove that kind of post from the cache table directly. @jeawhanlee Is it doable without much complexity?

@jeawhanlee
Copy link
Contributor Author

@Mai-Saad 2 and 3 are okay.

For the first one, it's acceptable, but it would be better if we could remove that kind of post from the cache table directly. @jeawhanlee Is it doable without much complexity?

@piotrbak We should be able to do that.

@piotrbak
Copy link
Contributor

@jeawhanlee Please try to do that. If you have any troubles or feel it can introduce side effects, please ping me

@Mai-Saad
Copy link
Contributor

@jeawhanlee Thanks for the updates. editing public post to private, is not sending any requests now.
However, we are sending requests to the URL in the following cases:
1- Update while having pending private post => When we remove the private URL from the cache table, we send a request (404)
2- While using a custom sitemap, containing private URLs => when we remove the private URL, we are sending a request (404)
can you please check?

@jeawhanlee jeawhanlee requested a review from remyperona June 22, 2023 11:07
Copy link
Contributor

@engahmeds3ed engahmeds3ed left a comment

Choose a reason for hiding this comment

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

After a discussion with @jeawhanlee we agreed to make it more performant and use static variable to have the list of private urls not to get the list again with each url and instead of adding a new condition/login we will use the filter rocket_preload_exclude_urls to exclude all of those private posts urls.

@jeawhanlee jeawhanlee requested a review from engahmeds3ed June 26, 2023 09:32
@vmanthos vmanthos self-requested a review June 27, 2023 10:58
Copy link
Contributor

@vmanthos vmanthos left a comment

Choose a reason for hiding this comment

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

Working as expected. 👍

TestRail report: testrail-report-496.pdf

@vmanthos vmanthos added this to the 3.14.1 milestone Jun 27, 2023
@vmanthos vmanthos added this pull request to the merge queue Jun 27, 2023
Merged via the queue into develop with commit 252cead Jun 27, 2023
@vmanthos vmanthos deleted the enhancement/5885-do-not-preload-private-pages branch June 27, 2023 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: preload type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preload: Don't try to preload private pages
7 participants