Skip to content

Remove incorrect album_id filter from video theatre query#4904

Open
GaryJones wants to merge 2 commits intobuddyboss:releasefrom
GaryJones:fix/video-theatre-album-query
Open

Remove incorrect album_id filter from video theatre query#4904
GaryJones wants to merge 2 commits intobuddyboss:releasefrom
GaryJones:fix/video-theatre-album-query

Conversation

@GaryJones
Copy link
Copy Markdown

Problem

The video theatre AJAX handler in bp-templates/bp-nouveau/includes/video/ajax.php passes 'album_id' => 'existing-video' when querying for a specific video by its ID. The query layer interprets this non-numeric value as album_id = 0, which restricts results to videos that do not belong to any album.

This means that when a user clicks to view a video in theatre mode that is associated with an album, the query returns no results and playback fails silently.

Fix

Remove the album_id parameter from the query arguments. The query already filters by a specific video ID via the 'include' parameter, so the album_id restriction is redundant. Without it, videos are found regardless of whether they belong to an album or not.

Test plan

  • Upload a video to an album in a group or profile
  • Click the video to open it in theatre/lightbox mode — should play successfully
  • Verify that videos not in any album still play correctly in theatre mode
  • Verify that the theatre navigation (previous/next) works for album videos

The video theatre AJAX handler passes album_id => 'existing-video'
when querying for a specific video by ID. This value is interpreted
as album_id = 0 by the query, restricting results to videos that
do not belong to any album. Videos attached to an album are excluded,
breaking theatre playback for album-associated videos.

Since the query already filters by a specific video ID via the
'include' parameter, the album_id restriction is unnecessary and
counterproductive. Remove it so all videos are found regardless
of album association.
@chetansatasiya
Copy link
Copy Markdown
Contributor

@claude Review the PR

@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Claude finished @chetansatasiya's task in 1m 21s —— View job


PR Review

  • Gather context
  • Read changed file and diff
  • Analyze code change
  • Post review feedback

Summary

The fix is correct. The root cause is well-documented in the query layer at src/bp-video/classes/class-bp-video.php:447-451:

// existing-video check to query video which has no albums assigned.
if ( ! empty( $r['album_id'] ) && 'existing-video' !== $r['album_id'] ) {
    $where_conditions['album'] = "m.album_id = {$r['album_id']}";
} elseif ( ! empty( $r['album_id'] ) && 'existing-video' === $r['album_id'] ) {
    $where_conditions['album'] = 'm.album_id = 0';
}

Passing 'album_id' => 'existing-video' always generates WHERE m.album_id = 0, which means only videos not in any album are returned. The 'include' => $video_id parameter alone is sufficient to retrieve the specific video, regardless of whether it belongs to an album.


The Change

- 'album_id' => 'existing-video',
+ // Do not filter by album_id here. The 'existing-video' value
+ // restricts the query to videos with album_id = 0, which
+ // excludes videos that belong to an album.

Verdict: Correct fix. The removal is sound and the comment clearly explains the intent and the footgun for future maintainers. The 'existing-video' sentinel is used legitimately elsewhere in the codebase (when albums are disabled) but has no place in a query that is already scoped to a single video ID via include.


Minor Notes (Pre-existing Issues, Not This PR's Responsibility)

  1. Redundant null check at ajax.php:1524-1526empty( $attachment_id ) is already caught by the combined check at line 1520, so the second check is dead code:

    // Line 1520 already covers this:
    if ( empty( $video_id ) || empty( $attachment_id ) ) {
        wp_send_json_error( $response );
    }
    // Redundant:
    if ( empty( $attachment_id ) ) {
        wp_send_json_error( $response );
    }

    Fix this →

  2. Grammar typo at ajax.php:1581'uploaded an video' should be 'uploaded a video' (translatable string in the buddyboss text domain).
    Fix this →


Conclusion

The fix is minimal, correct, and appropriately scoped. The explanatory comment is a good addition given how non-obvious the 'existing-video' sentinel value is. The two pre-existing issues noted above are out of scope for this PR.

Approved.

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.

2 participants