Section API-controller actions #6955
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added index, show, and update actions for the api controller of the sections model. I modified routes.rb (to actually establish the existence of these routes). I realised that because of the MainApiController (which all the API controllers inherit), the SessionHandler.check_record method already ensures that an incoming request trying to do something with a particular record is valid (i.e. the record actually exists), so it would be redundant to check whether current_course.sections.find(params[:id]) for example returns a valid current_course.
Added rspec tests for the update, index, and show actions of the api controller of sections. I tested both authorised and unauthorised requests, and for the GET requests (show and index), where either a json or xml response is expected, I tested that all necessary keys (corresponding to model attributes) are present, although I did not check whether the values of these attributes match between the responses and the original object data (not sure whether I should go this far). Also, for the #index request, some of my expectations perform an equality comparison between arrays; maybe it would be preferable to instead convert these to sets and make a comparison between sets.
…tion update-with-upstream
Pull Request Test Coverage Report for Build 7939947237Details
💛 - Coveralls |
Turns out I had accidentally deleted my tests for #destroy. I also changed a put destroy to a put update (which is what it should be)
Updated changelog and utilized existing record method in the update method of api-controller for sections.
There was a problem hiding this comment.
@mimischly7 good work, the tests are good. I left two small comments
| end | ||
|
|
||
| def show | ||
| section = current_course.sections.find(params[:id]) |
There was a problem hiding this comment.
You can use record here as well
| old_name = section.name | ||
| put :update, params: { course_id: course.id, id: section.id, name: 'LEC999' } | ||
| expect(response).to have_http_status(403) | ||
| expect(section.name).to equal(old_name) # both equal and eq work here |
There was a problem hiding this comment.
This comment is not necessary
…tion merge-upstream
Using record instead of manually finding object, and removing redundant comment
Changelog.md
Outdated
| @@ -8,6 +8,7 @@ | |||
| - Fix bug where grader distribution graph displays graders multiple times (#6950) | |||
| - Fixed bug where TA Summary table failed to display members (#6949) | |||
| - Make display of group member information consistent across submissions and summary table (#6917) | |||
| - Add new routes for `update`, `show`, and `index` actions of the Sections API Controller (#6955) | |||
There was a problem hiding this comment.
Please do a merge from master, and then make sure this changelog entry is under [unreleased]
david-yz-liu
approved these changes
Feb 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Due to an instructor request, we are currently developing an API-controller for the
Section's model. This pull request takes care of some of the actions of this controller, as well as extensive testing of these new actions.Your Changes
Description:
Added
index,show, andupdateactions to the API controller of theSection's model. I modified routes.rb (to actually establish the existence of these routes). Added thoroughRSPECtests for these actions.Type of change (select all that apply):
Testing
I tested both authorized and unauthorized requests, and for the GET requests (show and index), where either a
jsonorxmlresponse is expected, I tested that all necessary keys (corresponding to model attributes) are present, although I did not check whether the values of these attributes match between the responses and the original object data (not sure whether I should go this far). Also, for theindexrequest, some of my expectations perform an equality comparison between arrays; maybe it would be preferable to instead convert these to sets and make a comparison between sets.Questions and Comments (if applicable)
I realized that because of the
MainApiController(which all the API controllers inherit), theSessionHandler.check_recordmethod already ensures that an incoming request trying to do something with a particular record is valid (e.g. the record actually exists), so it would be redundant to check this again in the controller; e.g. whethercurrent_course.sections.find(params[:id])returns a valid section.Checklist
Pull request to make documentation changes (if applicable)
MarkUsProject/Wiki#206