E2006. Refactor tree_display_controller.rb #1698
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
Rename children_node_ng to get_folder_contents. Change HTML method from POST to GET. Remove Tab System getter. Moved to folder contents getter Refactor children_node_ng Rename children_node_ng to get_folder_contents. Change HTML method from POST to GET. Remove Tab System getter. Moved to folder contents getter
Remove duplicate functions created by the merge.
…ildren_node_2_ng.
Note: This object is not a FolderNode, but has the necessary information to populate a FolderNode object.
Comments more clear
Condense functions and make names more clear
(cherry picked from commit ba6c360)
This removes course_node_for_current_ta? and assignment_node_for_current_ta?
Follows the pattern of serialize_folder_to_json
It doesn't seem to be called anywhere.
This new method will replace update_is_available() and update_is_available_2(). This optimization reduces redundant checks on the TA. It should not matter how the TA relates to the instructor. We should only check if the TA has privileges to view this course. If they do then all other checks are not needed.
assignment_or_course_is_available?() takes the place of these 2 methods.
Generated by expertiza-bot |
1 similar comment
|
Hey @pjloheide, |
PR bot doesn't not like these.
Instructors and TA's have access to courses, not individual assignments. It doesn't matter which node is passed in, we only about course access.
Required based off rubric.
Its true it should not redirect to '/tree_display/list', but its probably better to check that it directly renders #list by checking the http status code.
We will check the http status code like the instructor test since no redirect should occur.
This test doesn't use any methods from this class. It is not our job to test 'ta?()'.
Adding end to remove error
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.
Remove unnecessary methods - Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
Save code and avoid DRY issues - In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.
Improve understanding - For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.