|
|
Line 40: |
Line 40: |
| 3. Used a single update_attributes method in place of the many instance variables. | | 3. Used a single update_attributes method in place of the many instance variables. |
|
| |
|
| '''Before'''
| |
| <pre>
| |
| https://github.com/palvitgarg99/expertiza/commit/9e99f56c9f8051fa5ee6b6a4bdf3659ec50852dc
| |
| https://github.com/palvitgarg99/expertiza/commit/66fc8bc5fc3850639306ad2be79afaa3de1fad70
| |
| def update
| |
| @topic = SignUpTopic.find(params[:id])
| |
| if @topic
| |
| @topic.topic_identifier = params[:topic][:topic_identifier]
| |
| update_max_choosers @topic
| |
| @topic.category = params[:topic][:category]
| |
| @topic.topic_name = params[:topic][:topic_name]
| |
| @topic.micropayment = params[:topic][:micropayment]
| |
| @topic.description = params[:topic][:description]
| |
| @topic.link = params[:topic][:link]
| |
| @topic.save
| |
| params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
| |
| undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
| |
| else
| |
| flash[:error] = "The topic could not be updated."
| |
| end
| |
| # Akshay - correctly changing the redirection url to topics tab in edit assignment view.
| |
| redirect_to edit_assignment_path(params[:assignment_id]) + "#tabs-2"
| |
| end
| |
| </pre>
| |
|
| |
|
| '''After'''
| |
| <pre>
| |
| def update
| |
| @topic = SignUpTopic.find(params[:id])
| |
| if @topic
| |
| update_max_choosers @topic
| |
| updated_max_choosers = @topic.max_choosers
| |
| @topic.update_attributes(topic_identifier: params[:topic][:topic_identifier], max_choosers: updated_max_choosers, category: params[:topic][:category], topic_name: params[:topic][:topic_name], micropayment: params[:topic][:micropayment], description: params[:topic][:description], link: params[:topic][:link])
| |
| undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
| |
| else
| |
| flash[:error] = "The topic could not be updated."
| |
| end
| |
| # Akshay - correctly changing the redirection url to topics tab in edit assignment view.
| |
| redirect_to edit_assignment_path(params[:assignment_id]) + "#tabs-2"
| |
| end
| |
| </pre>
| |
|
| |
|
| 4. The requirement that deadlines be submitted is no longer implemented and therefore a separate function for "staggered" is no longer needed. | | 4. The requirement that deadlines be submitted is no longer implemented and therefore a separate function for "staggered" is no longer needed. |
Line 97: |
Line 57: |
| 8. Refactored the participants variable and improved readability. | | 8. Refactored the participants variable and improved readability. |
|
| |
|
| '''Before'''
| |
| <pre>
| |
| @participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
| |
| </pre>
| |
|
| |
| '''After'''
| |
| <pre>
| |
| ip = session[:ip]
| |
| @participants = SignedUpTeam.find_team_participants(@id, ip)
| |
| </pre>
| |
|
| |
|
| 9. Previously signup_as_instructor_action provided unhelpful feedback to the instructor by only stating if the instructor could/couldn't assign a topic for a student. The refactored code provides the instructor with the student id of the person that they are assigning, the team that the student is on, and the topic that the instructor is assigning to the team. | | 9. Previously signup_as_instructor_action provided unhelpful feedback to the instructor by only stating if the instructor could/couldn't assign a topic for a student. The refactored code provides the instructor with the student id of the person that they are assigning, the team that the student is on, and the topic that the instructor is assigning to the team. |
|
| |
|
| '''Before'''
| |
| <pre>
| |
| def signup_as_instructor_action
| |
| user = User.find_by(name: params[:username])
| |
| if user.nil? # validate invalid user
| |
| flash[:error] = "That student does not exist!"
| |
| else
| |
| if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
| |
| if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
| |
| flash[:success] = "You have successfully signed up the student for the topic!"
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up student for topic: ' + params[:topic_id].to_s)
| |
| else
| |
| flash[:error] = "The student has already signed up for a topic!"
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a student who already has a topic')
| |
| end
| |
| else
| |
| flash[:error] = "The student is not registered for the assignment!"
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'The student is not registered for the assignment: ' << user.id)
| |
| end
| |
| end
| |
| redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
| |
| end
| |
| </pre>
| |
|
| |
|
| '''After'''
| |
| <pre>
| |
| def signup_as_instructor_action
| |
| #put name of student and team in log
| |
| # flash name of student and team
| |
| user = User.find_by(name: params[:username])
| |
| team = Team.find_team_for_assignment_and_user(params[:assignment_id], user.id).first
| |
| assignment = Assignment.find(params[:assignment_id])
| |
| if user.nil? # validate invalid user
| |
| flash[:error] = user.name + " does not exist!"
| |
| else #If the user is not null and the student exists then sign up student for topic
| |
| if AssignmentParticipant.exists? user_id: user.id, parent_id: params[:assignment_id]
| |
| if SignUpSheet.signup_team(params[:assignment_id], user.id, params[:topic_id])
| |
| flash[:success] = "You have successfully signed up " + user.name + " on " + team.name + " for the topic " + params[:topic_id].to_s
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor signed up ' + user.name + ' on team ' + team.name + ' for topic: ' + params[:topic_id].to_s)
| |
| else #If the students team already has a topic then cancel the sign up
| |
| flash[:error] = user.name + " on " + team.name + " has already signed up for a topic!"
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, '', 'Instructor is signing up a ' + user.name + 'on ' + team.name + ' who already has a topic')
| |
| end
| |
| else #If the student isn't a part of the assignment then cancel the sign up
| |
| flash[:error] = user.name + " is not registered for the assignment!"
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, '', user.name + ' is not registered for the assignment: ' + assignment.name)
| |
| end
| |
| end
| |
| redirect_to controller: 'assignments', action: 'edit', id: params[:assignment_id]
| |
| end
| |
| </pre>
| |
|
| |
|
| 10. delete_signup and delete_signup_as_instructor violated the DRY principle due to both functions using identical if/else statements. To solve this issue, delete_signup_heleper function was created. | | 10. delete_signup and delete_signup_as_instructor violated the DRY principle due to both functions using identical if/else statements. To solve this issue, delete_signup_heleper function was created. |
|
| |
|
| '''Before'''
| |
| <pre>
| |
| def delete_signup
| |
| participant = AssignmentParticipant.find(params[:id])
| |
| assignment = participant.assignment
| |
| drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
| |
| # A student who has already submitted work should not be allowed to drop his/her topic!
| |
| # (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
| |
| # If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
| |
| # If there is a drop topic deadline, student cannot drop topic after this deadline.
| |
| if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
| |
| flash[:error] = "You have already submitted your work, so you are not allowed to drop your topic."
| |
| ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for already submitted a work: ' + params[:topic_id].to_s)
| |
| elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
| |
| flash[:error] = "You cannot drop your topic after the drop topic deadline!"
| |
| ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping topic for ended work: ' + params[:topic_id].to_s)
| |
| else
| |
| delete_signup_for_topic(assignment.id, params[:topic_id], session[:user].id)
| |
| flash[:success] = "You have successfully dropped your topic!"
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
| |
| end
| |
| redirect_to action: 'list', id: params[:id]
| |
| end
| |
|
| |
| def delete_signup_as_instructor
| |
| # find participant using assignment using team and topic ids
| |
| team = Team.find(params[:id])
| |
| assignment = Assignment.find(team.parent_id)
| |
| user = TeamsUser.find_by(team_id: team.id).user
| |
| participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
| |
| drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
| |
| if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
| |
| flash[:error] = "The student has already submitted their work, so you are not allowed to remove them."
| |
| ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for already submitted work: ' + params[:topic_id].to_s)
| |
| elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
| |
| flash[:error] = "You cannot drop a student after the drop topic deadline!"
| |
| ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Drop failed for ended work: ' + params[:topic_id].to_s)
| |
| else
| |
| delete_signup_for_topic(assignment.id, params[:topic_id], participant.user_id)
| |
| flash[:success] = "You have successfully dropped the student from the topic!"
| |
| ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Student has been dropped from the topic: ' + params[:topic_id].to_s)
| |
| end
| |
| redirect_to controller: 'assignments', action: 'edit', id: assignment.id
| |
| end
| |
| </pre>
| |
|
| |
| '''After'''
| |
| <pre>
| |
| def delete_signup_helper(isInstructor, participant, assignment, drop_topic_deadline)
| |
| messages = ["You have already submitted your work, so you are not allowed to drop your topic.",
| |
| "You cannot drop your topic after the drop topic deadline!",
| |
| "You have successfully dropped your topic!",
| |
| "The team has already submitted their work, so you are not allowed to remove them.",
| |
| "You cannot drop a student after the drop topic deadline!",
| |
| "You have successfully dropped the student from the topic!"
| |
| ]
| |
| if isInstructor == false
| |
| flash1 = messages[0]
| |
| flash2 = messages[1]
| |
| flash3 = messages[2]
| |
| id_param = session[:user]
| |
| else
| |
| flash1 = messages[3]
| |
| flash2 = messages[4]
| |
| flash3 = messages[5]
| |
| id_param = participant
| |
| end
| |
|
| |
| if !participant.team.submitted_files.empty? or !participant.team.hyperlinks.empty?
| |
| flash[:error] = flash1
| |
| ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping failed for already submitted a work: ' + params[:topic_id].to_s)
| |
| elsif !drop_topic_deadline.nil? and Time.now > drop_topic_deadline.due_at
| |
| flash[:error] = flash2
| |
| ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].id, 'Dropping failed for ended work: ' + params[:topic_id].to_s)
| |
| else
| |
| delete_signup_for_topic(assignment.id, params[:topic_id], id_param.id)
| |
| flash[:success] = flash3
| |
| ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].id, 'Student has dropped the topic: ' + params[:topic_id].to_s)
| |
| end
| |
| end
| |
|
| |
| # this function is used to delete a previous signup
| |
| def delete_signup
| |
| participant = AssignmentParticipant.find(params[:id])
| |
| assignment = participant.assignment
| |
| drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
| |
| # A student who has already submitted work should not be allowed to drop his/her topic!
| |
| # (A student/team has submitted if participant directory_num is non-null or submitted_hyperlinks is non-null.)
| |
| # If there is no drop topic deadline, student can drop topic at any time (if all the submissions are deleted)
| |
| # If there is a drop topic deadline, student cannot drop topic after this deadline.
| |
| delete_signup_helper(false, participant, assignment, drop_topic_deadline)
| |
| redirect_to action: 'list', id: params[:id]
| |
| end
| |
|
| |
| def delete_signup_as_instructor
| |
| # find participant using assignment using team and topic ids
| |
| team = Team.find(params[:id])
| |
| assignment = Assignment.find(team.parent_id)
| |
| user = TeamsUser.find_by(team_id: team.id).user
| |
| participant = AssignmentParticipant.find_by(user_id: user.id, parent_id: assignment.id)
| |
| drop_topic_deadline = assignment.due_dates.find_by(deadline_type_id: 6)
| |
| delete_signup_helper(true, participant, assignment, drop_topic_deadline)
| |
| redirect_to controller: 'assignments', action: 'edit', id: assignment.id
| |
| end
| |
| </pre>
| |
|
| |
|
| == Testing == | | == Testing == |
This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
Introduction
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.
Peer Review Information
For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:
- Instructor login: username -> instructor6, password -> password
- Student login: username -> student5899, password -> password
About Controller
The sign up sheet controller
- Allows an instructor to add/remove topics
- Allows an instructor to assign/remove students to topics
- Allows an student to see the list of available topics which can be bid on for given OSS assignment.
Issues
1. Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well.
2. Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
3. Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
4. What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
5. The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
6. Signup_as_instructor_action has if-else ladder. It can be made more elegant.
7. Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. Refactor.
Solutions
1. Changed the name of the controller from sign_up_sheet_controller.rb --> signup_sheet_controller.rb. Also changed several method names that had 'sign_up' being used as an adjective or a noun to 'signup'.
https://github.com/palvitgarg99/expertiza/commit/9e99f56c9f8051fa5ee6b6a4bdf3659ec50852dc
2. Changed the create method to only create a new topic if that topic doesn't already exist. If it does exist, the program flashes an error message and backs out.
3. Used a single update_attributes method in place of the many instance variables.
4. The requirement that deadlines be submitted is no longer implemented and therefore a separate function for "staggered" is no longer needed.
5. Ad_info was renamed to ads_for_partners_info to give better picture of what function does.
https://github.com/palvitgarg99/expertiza/commit/6507f15f6d23f44900465536a91cc6c80f90ad75
6. signup_as_instructor is not needed and has been removed.
7. The list method is broken into 2 functions. The small part of computing signup topics is created. Also comments has been added to understand the functio better.
https://github.com/palvitgarg99/expertiza/commit/4b7bb6113dfb8984d7aa4dca467b55f67de3c93a
https://github.com/palvitgarg99/expertiza/commit/bd86f5a4ec8aa02c27a0cd34c85bd21776feca2c
https://github.com/palvitgarg99/expertiza/commit/56be9eed8ac89632041608947108f87dfbcb28ab
8. Refactored the participants variable and improved readability.
9. Previously signup_as_instructor_action provided unhelpful feedback to the instructor by only stating if the instructor could/couldn't assign a topic for a student. The refactored code provides the instructor with the student id of the person that they are assigning, the team that the student is on, and the topic that the instructor is assigning to the team.
10. delete_signup and delete_signup_as_instructor violated the DRY principle due to both functions using identical if/else statements. To solve this issue, delete_signup_heleper function was created.
Testing
- RSpec
- signup_as_instructor_action is the only function updated. All other functions has remained the same. To run the spec file, run:
- rspec spec/controllers/signup_sheet_controller_spec.rb
- Physical Testing
- Login as instructor
- Hover on Manage Tab
- Click on Assignments Tab
- Click on Edit Button for "OSS project & documentation" assignment
- Select the Topic Tab
- Click the Green Check for any topic
- Enter a student UnityID and click submit
- Login as student
- Click on OSS project/Writing assignment 2
- Click on Signup sheet
- The list must be visible, which indiactes functionality is as before after refactoring list fucntion.