Skip to content

PROD-10321 - #5032

Open
jitendrabanjara1991 wants to merge 2 commits into
releasefrom
PROD-10321
Open

PROD-10321#5032
jitendrabanjara1991 wants to merge 2 commits into
releasefrom
PROD-10321

Conversation

@jitendrabanjara1991

Copy link
Copy Markdown
Contributor

Jira Issue:

https://buddyboss.atlassian.net/browse/PROD-9951
https://buddyboss.atlassian.net/browse/PROD-10321

General Note

Keep all conversations related to this PR in the associated Jira issue(s). Do NOT add comment on this PR or edit this PR’s description.

Notes to Developer

  • Ensure the IDs (i.e. PROD-1) of all associated Jira issues are reference in this PR’s title
  • Ensure that you have achieved the Definition of Done before submitting for review
  • When this PR is ready for review, move the associate Jira issue(s) to “Needs Review” (or “Code Review” for Dev Tasks)

Notes to Reviewer

  • Ensure that the Definition of Done have been achieved before approving a PR
  • When this PR is approved, move the associated Jira issue(s) to “Needs QA” (or “Approved” for Dev Tasks)

…s against live avatar and cover state

The staleness check in bp_xprofile_get_selected_options_user_progress()
compared bb_validate_gravatar() — always false, since its transient is
never set on this path — against the stored is_uploaded flag. Every
member with an uploaded avatar mismatched permanently and paid a full
meter recalculation on every widget render, while a stale 0 (avatar
detection unavailable while an offload/CDN plugin was inactive) never
recalculated at all.

Compare the cached is_uploaded flags against live avatar and cover
state instead, recalculating only on genuine drift. Gravatar
disambiguation is kept for the stored=1/no-uploaded-avatar subset so
gravatar users behave as before; the outbound gravatar probe stays
confined to the recalculation itself.

Ship together with buddyboss-offload-media (detection filter + purge
migration, 0f0d1eab) — its bp_get_user_has_avatar filter is how
offloaded avatars register as live state here.
@buddyboss buddyboss deleted a comment from claude Bot Aug 20, 2026
@buddyboss buddyboss deleted a comment from claude Bot Aug 20, 2026
…s against live avatar and cover state

The staleness check in bp_xprofile_get_selected_options_user_progress()
compared bb_validate_gravatar() — always false, since its transient is
never set on this path — against the stored is_uploaded flag. Every
member with an uploaded avatar mismatched permanently and paid a full
meter recalculation on every widget render, while a stale 0 (avatar
detection unavailable while an offload/CDN plugin was inactive) never
recalculated at all.

Compare the cached is_uploaded flags against live avatar and cover
state instead, recalculating only on genuine drift. Gravatar handling
honors the recalculation's own counting gate (profile gravatars
enabled, non-blank default) and detects newly-countable gravatars from
the cached probe headers only when they carry the markers the
recalculation itself requires (200 + Link), so both directions of a
gravatar setting toggle heal in one view without recalculation loops.
The outbound gravatar probe stays confined to the recalculation.

Ship together with buddyboss-offload-media (detection filter + purge
migration) — its bp_get_user_has_avatar filter is how offloaded
avatars register as live state here.
@buddyboss buddyboss deleted a comment from claude Bot Aug 20, 2026
@buddyboss buddyboss deleted a comment from claude Bot Aug 20, 2026
@jitendrabanjara1991

Copy link
Copy Markdown
Contributor Author

@claude
Please review from scratch.. Ignore OLD review, make sure not break any existing logic

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Claude finished @jitendrabanjara1991's task in 2m 26s —— View job


Claude finished reviewing this PR (from scratch)

Todo list

  • Gather context (diff, changed file, both commits)
  • Understand the request (fresh review, ignore old review)
  • Trace all functions referenced by the new staleness logic (bp_get_user_has_avatar, bp_attachments_get_user_has_cover_image, bb_validate_gravatar, bp_xprofile_get_user_progress, transient key used by bp_core_fetch_avatar)
  • Post review feedback

Note: I was not able to run composer lint-php / phpcs / php -l in this sandbox (blocked by tool permissions), so the notes below are from manual reading only. Please run composer lint-php before merge to confirm formatting/WPCS compliance.


Summary

src/bp-core/bp-core-functions.phpbp_xprofile_get_selected_options_user_progress() (around line 5237)

This replaces a broken staleness check (bb_validate_gravatar() compared against the stored is_uploaded flag — a comparison that would basically always fail for uploaded-avatar users and never catch legitimately stale "0" values) with a proper live-vs-cached comparison for both profile photo and cover photo.

I traced every function/cache-key this touches against its real implementation:

  • bp_get_user_has_avatar() is called with no_grav => true, so it hits bp_core_fetch_avatar()'s early-return filesystem path and never triggers the outbound gravatar HTTP probe — the "cheap check" claim in the comment holds up.
  • The gravatar gate (bp_enable_profile_gravatar() && 'blank' !== get_option( 'avatar_default', 'mystery' )) matches exactly what bp_xprofile_get_user_progress() (the actual recalculation, src/bp-xprofile/bp-xprofile-functions.php:2849) itself checks before counting a gravatar.
  • The transient key base64_encode( 'https://www.gravatar.com/avatar/' . md5( strtolower( $email ) ) . '?d=404' ) matches the key bp_core_fetch_avatar() writes to (src/bp-core/bp-core-avatars.php:692) and that bb_validate_gravatar() reads — same format (plain get_headers() array, not the associative form), so the header-scan loop for a Link: entry is reading real data, not guessing at a shape.
  • bp_attachments_get_user_has_cover_image() is filesystem-based, no network call.

So the core logic is sound and each "cheap check only" claim in the docblock/comment is actually true against the current codebase — this isn't just narrative.

Minor observations (non-blocking)

  1. Intentional asymmetry between the two gravatar directions (lines ~5286 vs ~5296): the stored=1, live=0 direction still calls bb_validate_gravatar() (loose preg_match('|200|', ...)), while the stored=0, live=0 direction requires an exact 'HTTP/1.1 200 OK' === $headers[0] + Link header match. This is called out in the comment as deliberate (mirrors old behavior for the first case, mirrors the recalculation's own predicate for the second to avoid a self-disagreeing loop). Worth a maintainer sanity check, but I don't see a bug — just flagging so it isn't mistaken for an oversight in future edits.
  2. Two duplicate-message commits (53abda1, 524ac14) — the second supersedes the first with a slightly different implementation (tighter gravatar gate). Might be worth squashing before merge, but doesn't affect correctness of HEAD.
  3. Local variable prefix bb_pc_ is unconventional (CLAUDE.md's bb_/bp_/bbp_ prefix convention is for functions/classes, not locals) but harmless — arguably improves readability given how many booleans are in play here.
  4. Behavior depends on a companion change in buddyboss-offload-media (per commit message) for offloaded avatars to register correctly via a bp_get_user_has_avatar filter. That's an external repo I can't verify from here — worth confirming the two ship together as intended.

Verdict

No correctness, security, or regression issues found. The new logic is more conservative than the old broken check in every direction (falls back to "not stale" / defers to the next natural recalculation trigger rather than guessing), and each cache-format/gate assumption checks out against the actual implementations in this codebase.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant