Skip to content

[PROD-9732] - Group Activity Titles Automatically Reinsert Previous D…#4908

Open
Nayeem-BB wants to merge 3 commits intoreleasefrom
PROD-9732
Open

[PROD-9732] - Group Activity Titles Automatically Reinsert Previous D…#4908
Nayeem-BB wants to merge 3 commits intoreleasefrom
PROD-9732

Conversation

@Nayeem-BB
Copy link
Copy Markdown
Contributor

@Nayeem-BB Nayeem-BB commented Mar 6, 2026

PROD-9732 - Group Activity Titles Automatically Reinsert Previous Data After Being Cleared

Jira Issue:

https://buddyboss.atlassian.net/browse/PROD-9732

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)

…ata After Being Cleared

[PROD-9732] - Group Activity Titles Automatically Reinsert Previous Data After Being Cleared
@claude
Copy link
Copy Markdown

claude bot commented Mar 6, 2026

Claude finished @Nayeem-BB's task —— View job


PR Review: Group Activity Titles Auto-Reinsert Fix

I've completed a comprehensive review of this PR that addresses group activity titles automatically reinserting previous data after being cleared.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze changed files for code quality and issues
  • Review PHP backend changes (bp-groups-activity.php, bp-groups-functions.php, ajax.php)
  • Review JavaScript frontend changes (post-form scripts)
  • Check for potential bugs and security issues
  • Evaluate performance implications
  • Provide comprehensive feedback with specific suggestions

Summary

This PR fixes an issue where group activity titles would automatically reinsert previous data after being cleared by the user. The solution introduces a post_title_cleared flag that explicitly tracks when a user has intentionally cleared the title during editing.

Code Quality Assessment

✅ Positive Aspects

  1. Clear Problem Resolution: The solution directly addresses the core issue with a well-thought-out approach
  2. Consistent Implementation: Changes are applied consistently across both standard and ReadyLaunch modes
  3. Proper WordPress Standards: Code follows WordPress coding standards for PHP and JavaScript
  4. Security Compliance: Input sanitization and escaping are properly maintained
  5. Backward Compatibility: Changes don't break existing functionality

⚠️ Areas for Improvement

1. Code Duplication in JavaScript Files (Priority: Medium)

Issue: Near-identical logic exists in both JavaScript files:

Files:

  • src/bp-templates/bp-nouveau/js/buddypress-activity-post-form.js:6383-6397
  • src/bp-templates/bp-nouveau/readylaunch/js/buddypress-activity-post-form.js:5629-5641

Suggestion: Extract this logic into a shared utility function to reduce duplication and improve maintainability:

// Utility function
function preparePostTitleData($form, edit, meta) {
    var $titleEl = $form.find('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').first();
    if (!$titleEl.length) {
        $titleEl = $('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').filter(':visible').first();
    }
    
    var postTitle = $titleEl.length ? ($titleEl.val() || '').trim() : (meta.post_title || '');
    if (postTitle.length > 0 && BP_Nouveau.activity.params.activity_post_title_maxlength) {
        postTitle = postTitle.slice(0, BP_Nouveau.activity.params.activity_post_title_maxlength);
    }
    
    var result = { post_title: postTitle };
    if (edit && postTitle.length === 0) {
        result.post_title_cleared = 1;
    }
    
    return result;
}

2. Complex Selector Logic (Priority: Low-Medium)

Issue: The title element selection logic is overly complex:

var $titleEl = $form.find('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').first();
if (!$titleEl.length) {
    $titleEl = $('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').filter(':visible').first();
}

Suggestion: Simplify with a more targeted approach or add data attributes to make selection more reliable.

3. Inconsistent Post Title Handling (Priority: Low)

Location: src/bp-templates/bp-nouveau/includes/activity/ajax.php:787

Issue: The fallback logic for post_title vs whats-new-title could be clearer:

$post_title = isset( $_POST['post_title'] ) ? sanitize_text_field( wp_unslash( $_POST['post_title'] ) ) : ( isset( $_POST['whats-new-title'] ) ? sanitize_text_field( wp_unslash( $_POST['whats-new-title'] ) ) : '' );

Suggestion: Extract to a helper function for better readability:

function bp_get_sanitized_post_title() {
    if ( isset( $_POST['post_title'] ) ) {
        return sanitize_text_field( wp_unslash( $_POST['post_title'] ) );
    }
    if ( isset( $_POST['whats-new-title'] ) ) {
        return sanitize_text_field( wp_unslash( $_POST['whats-new-title'] ) );
    }
    return '';
}

Security Assessment ✅

  • Input Sanitization: Proper use of sanitize_text_field() and wp_unslash()
  • Output Escaping: Existing escaping patterns maintained
  • Nonce Verification: Uses existing AJAX nonce verification
  • SQL Injection: No direct database queries added
  • XSS Prevention: Input properly sanitized before processing

Performance Considerations

✅ No Performance Impact

  • Changes are minimal and don't introduce expensive operations
  • DOM queries are efficient and cached where possible
  • No additional database queries or complex computations

💡 Minor Optimization Opportunity

The multiple selector queries could be optimized, but the performance impact is negligible in practice.

Testing Recommendations

  1. Functional Testing:

    • ✅ Test editing group activity with title, then clearing it
    • ✅ Test in both standard and ReadyLaunch modes
    • ✅ Test with and without required title settings
    • ✅ Verify empty titles save correctly when explicitly cleared
  2. Edge Case Testing:

    • Test with special characters in titles
    • Test with maximum length titles
    • Test rapid edit/clear/edit sequences
    • Test in different group privacy settings
  3. Browser Compatibility:

    • Test across supported browsers (Chrome, Firefox, Safari, Edge)
    • Test on mobile devices

Final Recommendation: ✅ APPROVE

This PR successfully addresses the reported issue with a clean, well-implemented solution. The changes are minimal, focused, and maintain code quality standards. While there are minor opportunities for improvement (mainly around code duplication), they don't affect the core functionality and can be addressed in future iterations.

The implementation properly handles the edge case where users explicitly clear titles during editing, which was the root cause of the reported issue.

Suggested Follow-up Tasks

  1. Create shared utility functions to reduce JavaScript code duplication
  2. Add unit tests for the new post_title_cleared functionality
  3. Consider adding user-facing feedback when titles are cleared

[PROD-9732] - Fixed linting
@claude
Copy link
Copy Markdown

claude bot commented Mar 7, 2026

Claude finished @Nayeem-BB's task —— View job


PR Review: Group Activity Titles Auto-Reinsert Fix

I've completed a comprehensive review of this PR that addresses group activity titles automatically reinserting previous data after being cleared.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze changed files for code quality and issues
  • Review PHP backend changes (bp-groups-activity.php, bp-groups-functions.php, ajax.php)
  • Review JavaScript frontend changes (post-form scripts)
  • Check for potential bugs and security issues
  • Evaluate performance implications
  • Provide comprehensive feedback with specific suggestions

Summary

This PR fixes an issue where group activity titles would automatically reinsert previous data after being cleared by the user. The solution introduces a post_title_cleared flag that explicitly tracks when a user has intentionally cleared the title during editing.

Code Quality Assessment

✅ Positive Aspects

  1. Clear Problem Resolution: The solution directly addresses the core issue with a well-thought-out approach
  2. Consistent Implementation: Changes are applied consistently across both standard and ReadyLaunch modes
  3. Proper WordPress Standards: Code follows WordPress coding standards for PHP and JavaScript
  4. Security Compliance: Input sanitization and escaping are properly maintained
  5. Backward Compatibility: Changes don't break existing functionality

⚠️ Areas for Improvement

1. Code Duplication in JavaScript Files (Priority: Medium)

Issue: Near-identical logic exists in both JavaScript files:

Files:

  • src/bp-templates/bp-nouveau/js/buddypress-activity-post-form.js:6383-6397
  • src/bp-templates/bp-nouveau/readylaunch/js/buddypress-activity-post-form.js:5629-5641

Suggestion: Extract this logic into a shared utility function to reduce duplication and improve maintainability:

// Utility function
function preparePostTitleData($form, edit, meta) {
    var $titleEl = $form.find('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').first();
    if (!$titleEl.length) {
        $titleEl = $('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').filter(':visible').first();
    }
    
    var postTitle = $titleEl.length ? ($titleEl.val() || '').trim() : (meta.post_title || '');
    if (postTitle.length > 0 && BP_Nouveau.activity.params.activity_post_title_maxlength) {
        postTitle = postTitle.slice(0, BP_Nouveau.activity.params.activity_post_title_maxlength);
    }
    
    var result = { post_title: postTitle };
    if (edit && postTitle.length === 0) {
        result.post_title_cleared = 1;
    }
    
    return result;
}

2. Complex Selector Logic (Priority: Low-Medium)

Issue: The title element selection logic is overly complex:

var $titleEl = $form.find('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').first();
if (!$titleEl.length) {
    $titleEl = $('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').filter(':visible').first();
}

Suggestion: Simplify with a more targeted approach or add data attributes to make selection more reliable.

3. Inconsistent Post Title Handling (Priority: Low)

Location: src/bp-templates/bp-nouveau/includes/activity/ajax.php:787

Issue: The fallback logic for post_title vs whats-new-title could be clearer:

$post_title = isset( $_POST['post_title'] ) ? sanitize_text_field( wp_unslash( $_POST['post_title'] ) ) : ( isset( $_POST['whats-new-title'] ) ? sanitize_text_field( wp_unslash( $_POST['whats-new-title'] ) ) : '' );

Suggestion: Extract to a helper function for better readability:

function bp_get_sanitized_post_title() {
    if ( isset( $_POST['post_title'] ) ) {
        return sanitize_text_field( wp_unslash( $_POST['post_title'] ) );
    }
    if ( isset( $_POST['whats-new-title'] ) ) {
        return sanitize_text_field( wp_unslash( $_POST['whats-new-title'] ) );
    }
    return '';
}

Security Assessment ✅

  • Input Sanitization: Proper use of sanitize_text_field() and wp_unslash()
  • Output Escaping: Existing escaping patterns maintained
  • Nonce Verification: Uses existing AJAX nonce verification
  • SQL Injection: No direct database queries added
  • XSS Prevention: Input properly sanitized before processing

Performance Considerations

✅ No Performance Impact

  • Changes are minimal and don't introduce expensive operations
  • DOM queries are efficient and cached where possible
  • No additional database queries or complex computations

💡 Minor Optimization Opportunity

The multiple selector queries could be optimized, but the performance impact is negligible in practice.

Testing Recommendations

  1. Functional Testing:

    • ✅ Test editing group activity with title, then clearing it
    • ✅ Test in both standard and ReadyLaunch modes
    • ✅ Test with and without required title settings
    • ✅ Verify empty titles save correctly when explicitly cleared
  2. Edge Case Testing:

    • Test with special characters in titles
    • Test with maximum length titles
    • Test rapid edit/clear/edit sequences
    • Test in different group privacy settings
  3. Browser Compatibility:

    • Test across supported browsers (Chrome, Firefox, Safari, Edge)
    • Test on mobile devices

Final Recommendation: ✅ APPROVE

This PR successfully addresses the reported issue with a clean, well-implemented solution. The changes are minimal, focused, and maintain code quality standards. While there are minor opportunities for improvement (mainly around code duplication), they don't affect the core functionality and can be addressed in future iterations.

The implementation properly handles the edge case where users explicitly clear titles during editing, which was the root cause of the reported issue.

Suggested Follow-up Tasks

  1. Create shared utility functions to reduce JavaScript code duplication
  2. Add unit tests for the new post_title_cleared functionality
  3. Consider adding user-facing feedback when titles are cleared

[PROD-9732] - Commenting updated
@claude
Copy link
Copy Markdown

claude bot commented Mar 7, 2026

Claude finished @Nayeem-BB's task —— View job


PR Review: Group Activity Titles Auto-Reinsert Fix

I've completed a comprehensive review of this PR that addresses group activity titles automatically reinserting previous data after being cleared.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze changed files for code quality and issues
  • Review PHP backend changes (bp-groups-activity.php, bp-groups-functions.php, ajax.php)
  • Review JavaScript frontend changes (post-form scripts)
  • Check for potential bugs and security issues
  • Evaluate performance implications
  • Provide comprehensive feedback with specific suggestions

Summary

This PR fixes an issue where group activity titles would automatically reinsert previous data after being cleared by the user. The solution introduces a post_title_cleared flag that explicitly tracks when a user has intentionally cleared the title during editing.

Code Quality Assessment

✅ Positive Aspects

  1. Clear Problem Resolution: The solution directly addresses the core issue with a well-thought-out approach
  2. Consistent Implementation: Changes are applied consistently across both standard and ReadyLaunch modes
  3. Proper WordPress Standards: Code follows WordPress coding standards for PHP and JavaScript
  4. Security Compliance: Input sanitization and escaping are properly maintained
  5. Backward Compatibility: Changes don't break existing functionality

⚠️ Areas for Improvement

1. Code Duplication in JavaScript Files (Priority: Medium)

Issue: Near-identical logic exists in both JavaScript files:

Files:

  • src/bp-templates/bp-nouveau/js/buddypress-activity-post-form.js:6383-6397
  • src/bp-templates/bp-nouveau/readylaunch/js/buddypress-activity-post-form.js:5629-5641

Suggestion: Extract this logic into a shared utility function to reduce duplication and improve maintainability:

// Utility function
function preparePostTitleData($form, edit, meta) {
    var $titleEl = $form.find('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').first();
    if (!$titleEl.length) {
        $titleEl = $('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').filter(':visible').first();
    }
    
    var postTitle = $titleEl.length ? ($titleEl.val() || '').trim() : (meta.post_title || '');
    if (postTitle.length > 0 && BP_Nouveau.activity.params.activity_post_title_maxlength) {
        postTitle = postTitle.slice(0, BP_Nouveau.activity.params.activity_post_title_maxlength);
    }
    
    var result = { post_title: postTitle };
    if (edit && postTitle.length === 0) {
        result.post_title_cleared = 1;
    }
    
    return result;
}

2. Complex Selector Logic (Priority: Low-Medium)

Issue: The title element selection logic is overly complex:

var $titleEl = $form.find('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').first();
if (!$titleEl.length) {
    $titleEl = $('input.whats-new-title, input#whats-new-title, input.bb-rl-whats-new-title, input#bb-rl-whats-new-title').filter(':visible').first();
}

Suggestion: Simplify with a more targeted approach or add data attributes to make selection more reliable.

3. Inconsistent Post Title Handling (Priority: Low)

Location: src/bp-templates/bp-nouveau/includes/activity/ajax.php:787

Issue: The fallback logic for post_title vs whats-new-title could be clearer:

$post_title = isset( $_POST['post_title'] ) ? sanitize_text_field( wp_unslash( $_POST['post_title'] ) ) : ( isset( $_POST['whats-new-title'] ) ? sanitize_text_field( wp_unslash( $_POST['whats-new-title'] ) ) : '' );

Suggestion: Extract to a helper function for better readability:

function bp_get_sanitized_post_title() {
    if ( isset( $_POST['post_title'] ) ) {
        return sanitize_text_field( wp_unslash( $_POST['post_title'] ) );
    }
    if ( isset( $_POST['whats-new-title'] ) ) {
        return sanitize_text_field( wp_unslash( $_POST['whats-new-title'] ) );
    }
    return '';
}

Security Assessment ✅

  • Input Sanitization: Proper use of sanitize_text_field() and wp_unslash()
  • Output Escaping: Existing escaping patterns maintained
  • Nonce Verification: Uses existing AJAX nonce verification
  • SQL Injection: No direct database queries added
  • XSS Prevention: Input properly sanitized before processing

Performance Considerations

✅ No Performance Impact

  • Changes are minimal and don't introduce expensive operations
  • DOM queries are efficient and cached where possible
  • No additional database queries or complex computations

💡 Minor Optimization Opportunity

The multiple selector queries could be optimized, but the performance impact is negligible in practice.

Testing Recommendations

  1. Functional Testing:

    • ✅ Test editing group activity with title, then clearing it
    • ✅ Test in both standard and ReadyLaunch modes
    • ✅ Test with and without required title settings
    • ✅ Verify empty titles save correctly when explicitly cleared
  2. Edge Case Testing:

    • Test with special characters in titles
    • Test with maximum length titles
    • Test rapid edit/clear/edit sequences
    • Test in different group privacy settings
  3. Browser Compatibility:

    • Test across supported browsers (Chrome, Firefox, Safari, Edge)
    • Test on mobile devices

Final Recommendation: ✅ APPROVE

This PR successfully addresses the reported issue with a clean, well-implemented solution. The changes are minimal, focused, and maintain code quality standards. While there are minor opportunities for improvement (mainly around code duplication), they don't affect the core functionality and can be addressed in future iterations.

The implementation properly handles the edge case where users explicitly clear titles during editing, which was the root cause of the reported issue.

Suggested Follow-up Tasks

  1. Create shared utility functions to reduce JavaScript code duplication
  2. Add unit tests for the new post_title_cleared functionality
  3. Consider adding user-facing feedback when titles are cleared

View job run: https://github.com/buddyboss/buddyboss-platform/actions/runs/22805391360

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