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.

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

RTO-35842 Allow multiple ids to show function #34

Merged

Conversation

krishnaeverestengineering
Copy link
Contributor

Ticket: https://jobready.atlassian.net/browse/RTO-35842

Update the moodle-rb gem to allow multiple ids to Courses show function.

README.md Outdated Show resolved Hide resolved
}
}
}.merge(query_options)
)
check_for_errors(response)

return response.parsed_response if response.parsed_response.count > 1
Copy link
Member

Choose a reason for hiding this comment

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

What does the Moodle API do if you call it with course IDs in the list that do not exist? Hopefully it will skip those and give us just the ones it has. If that's the case, it's possible we may only have 1, or even 0, in the response, even though the caller has passed multiple ids and is expecting an array back, so I think we should return an array (even of 0 or 1 item) if the user has passed more than one id.

Copy link
Member

Choose a reason for hiding this comment

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

I was actually going to suggest a change to this to a ternary statement, but then got sidetracked.

I do think we should check the number of ids passed in as described above.

jbramich
jbramich previously approved these changes Nov 14, 2024
:ids => {
'0' => id
}
id: ids
Copy link
Member

@karlcloudy karlcloudy Nov 14, 2024

Choose a reason for hiding this comment

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

This might be my fault as I probably got it wrong in the suggestion, but I've noticed the parameter key name here should be ids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

karlcloudy
karlcloudy previously approved these changes Nov 14, 2024
jbramich
jbramich previously approved these changes Nov 14, 2024
@karlcloudy karlcloudy dismissed stale reviews from jbramich and themself via e71f765 November 15, 2024 03:58
@krishnaeverestengineering krishnaeverestengineering merged commit e383398 into develop Nov 15, 2024
1 check passed
@krishnaeverestengineering krishnaeverestengineering deleted the RTO-35842-allow-multiple-ids-to-show-function branch November 15, 2024 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants