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.

Sign up for GitHub

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

Jump to bottom

GIT-718: Implement offset pagination for getRecordings (closes #718) #720

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

GIT-718: Implement offset pagination for getRecordings (closes #718) #720

wants to merge 5 commits into from

Conversation

git-lama
Copy link
Contributor

closes #718

@git-lama git-lama self-assigned this Oct 22, 2021
@git-lama git-lama requested a review from jfederico October 25, 2021 12:18
@git-lama git-lama added the needs work Code in this pull request needs to be updated label Oct 25, 2021
kepstin
kepstin requested changes Oct 25, 2021
View reviewed changes
app/controllers/bigbluebutton_api_controller.rb Outdated
@@ -268,7 +268,9 @@ def join
end

def get_recordings
if Rails.configuration.x.get_recordings_api_filtered && (params[:recordID].blank? && params[:meetingID].blank?)
limit = params[:limit] || Rails.configuration.x.default_pagination_limit
Copy link
Member

Choose a reason for hiding this comment

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

Need to have a max on the limit param - in discussion we chose 100 arbitrarily as the maximum limit. Also ensure that the limit is >0.

app/controllers/bigbluebutton_api_controller.rb
@@ -293,6 +295,9 @@ def get_recordings
query = query.with_recording_id_prefixes(params[:recordID].split(',')) if params[:recordID].present?
query = query.where(meeting_id: params[:meetingID].split(',')) if params[:meetingID].present?

offset = params[:offset] || 0
Copy link
Member

Choose a reason for hiding this comment

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

Validate that the provided offset param is ≥0

@git-lama git-lama removed the needs work Code in this pull request needs to be updated label Oct 25, 2021
@git-lama git-lama requested a review from kepstin October 25, 2021 20:33
@jfederico jfederico added this to the v1.3.1 milestone Oct 28, 2021
@jfederico jfederico changed the title GIT-718: Implement offset pagination for getRecordings GIT-718: Implement offset pagination for getRecordings (closes #718) Oct 29, 2021
@sonarcloud
Copy link

sonarcloud bot commented Nov 11, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kepstin
Copy link
Member

kepstin commented Nov 12, 2021

@fcecagno @paultrudel Before we merge support for the limit and offset params into scalelite, I just want to check with you about the error codes returned for invalid values.

This current scalelite PR is adding two new messageKey values, invalidParamLimit and invalidParamOffset - but this should match what we decide to merge into BigBlueButton itself.

@jfederico jfederico modified the milestones: v1.3.1, v1.4 Nov 17, 2021
@mabras
Copy link

mabras commented May 15, 2023

Can we see this soon?
Thank you @jfederico

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

@jfederico jfederico Awaiting requested review from jfederico

@kepstin kepstin Awaiting requested review from kepstin

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

Implement offset pagination for getRecordings
4 participants
@git-lama @kepstin @mabras @jfederico