PROD-10311

[jitendrabanjara1991]

Jira Issue:

https://buddyboss.atlassian.net/browse/PROD-10308
https://buddyboss.atlassian.net/browse/PROD-10311

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)

[claude]

Claude finished @jitendrabanjara1991's task in 6m 22s —— View job

---

Review complete — PROD-10311 (fresh review, ignoring prior review threads)

Todo list

• Gather context: fetch current PR diff vs origin/release
• Understand root cause and intended behavior (Bio field vs repeater sets)
• Verify the SQL-inserted BIO / preservation-on-save scenario described in the trigger comment
• Review each changed file in detail
• Validate normal add/edit/delete flows (non-repeater, repeater, empty/missing/existing/changed Bio)
• Check for regressions vs origin/release
• Post final review verdict

---

Root cause

The Bio field (type === 'biography') is not stored per-set like other profile fields — it mirrors the single WordPress wp_users.description value. A "repeater" field set clones its fields into multiple entries (e.g. "Work History" → Company #1, Company #2, …). If a Bio field were ever inside a repeater set, every clone would represent the same singleton WordPress value, which is an undefined/corrupt state. The PR's job is to make that state unreachable and to make sure that if it already exists (e.g. a row inserted straight into wp_bp_xprofile_fields via SQL, bypassing the app entirely), any later attempt to edit/save that field through the app doesn't silently accept or half-apply the invalid state — instead the save is refused outright so the pre-existing row is left untouched.

How the two commits work together

• Commit 1 (5c72684) blocks the field from entering a repeater set: BP_XProfile_Field::save() (class-bp-xprofile-field.php) and BP_XProfile_Group::save() (class-bp-xprofile-group.php) only refused the transition (new field / group move / repeater switched on).
• Commit 2 (2852754) closes the gap the trigger comment is about: class-bp-xprofile-field.php:472-474 now refuses every save of a Bio field while its group is repeater-enabled, not just the save that would newly place it there:

  if ( 'biography' === $this->type && bb_xprofile_is_repeater_group( $this->group_id ) ) {
      return false;
  }

  This check runs at the very top of save(), before any UPDATE/INSERT SQL is built or executed (verified by reading the full method, class-bp-xprofile-field.php:431-591). So when it fires, the DB row is left completely as-is — a true no-op, not a partial write.

SQL-inserted BIO scenario — traced end to end

1. A Bio field row is written directly via SQL into a group that already has is_repeater_enabled = on meta (bypassing all app-level guards, since raw SQL doesn't go through save()).
2. Admin opens Settings 2.0 → Profile Fields and edits that Bio field (rename, change description, toggle required, anything) and clicks save.
3. save_profile_field() (class-bb-admin-profile-fields-ajax.php:481) calls xprofile_insert_field( $args ) → BP_XProfile_Field::save() → the new guard hits → save() returns false before touching the DB → $saved_id is empty → the handler returns the friendly error (class-bb-admin-profile-fields-ajax.php:486-488) instead of wp_send_json_success.
4. Result: the pre-existing (SQL-inserted) Bio row is untouched — preserved exactly as it was. This matches the requirement precisely.

I also traced this beyond the Settings 2.0 AJAX path, since the guard lives in the model layer (BP_XProfile_Field::save()), not just the controller:

• BP_REST_XProfile_Fields_Endpoint::create_item() / update_item() (class-bp-rest-xprofile-fields-endpoint.php:490, :642) both call xprofile_insert_field() and correctly turn a falsy return into a WP_Error (500) rather than a fake success or a fatal.
• Any other/future caller of xprofile_insert_field() or new BP_XProfile_Field() → save() inherits the same protection automatically — this is a solid, DRY choke point.

Regression / side-effect check (things that must NOT be blocked)

• Editing other, unrelated fields in the same (invalid) repeater set — traced xprofile_update_clones_on_template_update() (bp-xprofile-repeaters.php:478, fired from xprofile_fields_saved_field): it only syncs clones tagged _cloned_from = $field->id for the field that was actually saved, via raw $wpdb->query() (not ->save()). Saving a sibling "Company" field never touches the Bio row. Pass.
• Reordering fields (drag/drop) — reorder_fields() (class-bb-admin-profile-fields-ajax.php:597) calls xprofile_update_field_position() → BP_XProfile_Field::update_position(), a separate raw-SQL path that never calls save(). Reordering a set containing a stuck Bio field still works. Pass.
• Deleting fields/groups — delete() methods are untouched by this diff. Pass.
• Non-Bio fields in non-repeater or repeater sets — guard is scoped to 'biography' === $this->type; nothing else is affected. Pass.
• Bio field in a normal (non-repeater) set — bb_xprofile_is_repeater_group() returns false → guard never triggers → normal save behavior, identical to origin/release. Pass.
• Escape hatch — turning the repeater off at the group level is never blocked (class-bp-xprofile-group.php's guard only fires on the off→on transition), so an admin can always recover a stuck set by disabling the repeater, after which the Bio field becomes editable/movable again. Pass, and matches the intent in the code comments.
• Empty/missing Bio — the guard depends only on type and the group's repeater state, never on field content (name/description/required), so empty-vs-populated Bio behaves identically. Pass.

Other things verified

• No duplicate function names introduced (bb_xprofile_is_repeater_group, bb_xprofile_group_has_bio_field are new and unique repo-wide).
• bp-xprofile-repeaters.php is loaded unconditionally in BP_XProfile_Component::includes(), so the two new helper functions are always defined before any request-time save() call — no function_exists() risk.
• bb_xprofile_group_has_bio_field() uses $wpdb->prepare() correctly; no SQL injection surface.
• Docblocks correctly use @since BuddyBoss [BBVERSION] per repo convention, Yoda conditions used consistently in the new code.
• I was not able to execute composer lint-php / phpcs / composer test in this sandbox (tool execution for those commands isn't available to me here), so this remains a static/manual trace rather than an executed-test run. I'd still recommend running these before merge if not already done in CI.

Remaining, non-blocking observations (carried forward / refined from prior context)

• No PHPUnit test covers the "existing invalid Bio-in-repeater row is preserved rather than overwritten on edit" scenario specifically — worth adding given this is now the core guarantee being requested here.
• The BP_REST_XProfile_Field_Groups_Endpoint::update_item() group-level gap noted in earlier review rounds on this PR (a REST caller can flip is_repeater_enabled to on on a group already containing Bio, bypassing BP_XProfile_Group::save()'s guard) is unchanged by this diff and lives in a file that is build-synced from the private buddyboss-platform-api repo, not edited here — consistent with the earlier conclusion that it's tracked there. Importantly, this diff's field-level guard now provides a second line of defense regardless: even if that REST call re-creates the invalid group state, any subsequent attempt to save the Bio field itself is still blocked by BP_XProfile_Field::save().

Final verdict: Approve

The fix correctly targets the root cause (Bio is a WordPress-singleton value that can't safely be cloned by a repeater) and, with the second commit, correctly guarantees that a Bio field already sitting in an invalid repeater-set state — however it got there, including direct SQL — is preserved unmodified on the next save rather than silently accepted or partially overwritten. I did not find any path that blocks a legitimate, unrelated save, and the escape hatch (disabling the repeater) keeps the set recoverable. Adding a regression test for the preservation scenario would be valuable but is not blocking.