Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-05-02T02:50:17Z

Abommag: /* UI */

==Introduction==

=== Problem Statement ===

This project adds a new feature which will enable a reviewer to send an Email to authors from inside Expertiza or for the author to email all those reviewing them. Often when reviewing another team’s submissions, a reviewer might notice that their deployment link is broken or there is some other issue. Currently, there is no way to email the authors. This features is proposed to solve this issue.

==Scope of the Feature==

- The feature allows authors to send a common Email to all the reviewers to report any issues or updates to their project to be reviewed. 
- It can allow a reviewer to send an email to a particular author if the reviewer wants to convey any issues found with that particular project. 
- Reviewers will be anonymous to authors. 

==Flowchart==

'''Author's View''' : The flow for authors to email reviewers regarding the work they have been reviewed for:

[[File:Author's view flowchart.jpg|1000px]] 

'''Reviewer's View''' : The flow for reviewers to email authors whose work they have reviewed:

[[File:Reviewers_View_Flowchart.jpg|1000px]] 

==UML Diagrams==

==='''Reviewers use case'''===

:: [[File:Reviewer_usecase.png|300px]] 

==='''Author's use case'''===

:: [[File:Author_usecase.png|300px]] 

==Proposed Changes==
===Code Implementation===

Expertiza code base already contains a module 'MailerHelper' which makes use of ActionMailer and contains methods to send emails to different users.
email_author_reviewers(subject, body, email) will be added which can be used in both the workflows to send an Email.
 
From the Reviewer's perspective, the following files will be updated and a new view will be created.

1. '''app/views/response/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/response_controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/response/author.html.erb :''' This is the new UI page. A mock up for the view is presented in the UI section. 
 

From Author's perspective, the following files will be updated and a new partial will be created to render the new view.

1. '''app/views/student_task/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/student_task controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/student_task/email_reviewers.html.erb :''' This is the new UI page. A mockup for the view is presented in the UI section. 

===UI===
As depicted below, Authors will be given a link on their main Assignment page “Send Email to Reviewers” which will take them to a new page where they are given a text box for typing and sending an email to all their reviewers. 

[[File:Assignment_page.png|800px]] 

[[File:reviewer_email.jpeg|800px]] 

Reviewers will be given a similar text box link on their “Other’s Work” page next to each review that they are doing. 

[[File:Author_SS.png|800px]] 

[[File:author_email.jpeg|800px]] 

==Implememtation==

The following are the screenshots of the changes that we made in the codebase.

===Send Email Method in MailerHelper===
We created a new method called send_mail_to_author_reviewers(subject, body, email), which accepts 3 parameters; subject, body and email. We used the same method for both sending emails to authors and reviewers.

[[File:Email_method.png|800px]] 

===Email from Reviewer to Author===
====View====
We modified the view to add a link that redirects the author to the form.

[[File:Email_the_author_link.png|1000px]] 

To create the author view, we created a new view called author.html.erb and created a form in that file, which asks the user to enter the subject and body.
[[File:Author_view.png|800px]] 

====Controller====
We created a method called send_email in the response_controller which handles the functionality for sending an email. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we pass the subject, body, and email from the view to the controller and send them to a method called send_mail_to_author_reviewers which is a part of MailerHelper. This method is responsible for sending emails to the author.

[[File:Send_email_author.png|1000px]] 

===Email from Author to Reviewers===
====View====
We modified the view to add a link that redirects the reviewer to the form.

[[File:Send_email_to_reviewers.png|1000px]] 

To create the reviewer view, we created a new view called email_reviewers.html.erb where we created a form that asks the user to enter the subject and body of the email.
[[File:Author_view.png|800px]] 

====Controller====
To send an email to all the reviewers, we created a method called send_email in the student_task_controller. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we go ahead and pass the parameters; subject, body, and reviewer.email to the send_mail_to_author_reviewers method from MailerHelper.

[[File:Send_email_reviewer.png|1000px]] 

==Testing==

===RSPEC Testing===

We tested the send_email method used in response_controller.rb and student_task_controller.rb which we have added to achieve the functionality of sending email to reviewers and authors. We have considered four scenarios which can occur while an email has to be sent, namely, user might not give the subject but the body, subject present but no body, both subject and body are present as desired in which case the email is sent on send email button click and the user is redirected to the student task list page and lastly, if there is no subject or body of the email is given, the user is flashed a message and is kept on the same page to fill in the desired fields. Below are the different test scenarios described above:

'''Test 1:'''
:: [[File:Test1.jpeg|800px]] 

'''Test 2:'''
:: [[File:Test2.jpeg|800px]] 

'''Test 3:'''
:: [[File:Test3.jpeg|800px]] 

'''Test 4:'''
:: [[File:Test4.jpeg|800px]] 

'''Test Result:''' We ran the test file using the command '''bundle exec rspec spec/controllers/response_controller_spec.rb''' and below is the test result.
:: [[File:Test result.jpeg|800px]] 

===Manual Testing===

For testing purposes, we are using the 'expertiza.debugging@gmail.com' as both author and reviewer.
There are two views that need to be tested manually.

1. Author View:

The author must follow the following steps:
1. Go to the Assignments page.
2. Click on the project for which they want to send email to the reviewers, in the student task view page then click on the right corner of the page where we have the link "send email to reviewers".
3. On clicking the above link, the author must be redirected to a page which will ask then for subject of email and text for the body of email.
4. Type the email subject, message and click on send.
5. Author can log in to the email to check if the message is received from receivers.

2. Reviewer View:

The reviewer must follow the following steps:
1. Go to the other's work page.
2. Next to every review that the reviewer is doing, will be a link on the right corner of the page called "send email to author".
3. On clicking the above link, the reviewer will be redirected to a page with text box for subject and message.
4. Type the email subject, message and click on send.
5. Log in to the email to check if the message is received.

==Future Scope==

1. Currently, only a subject and a text message are provided to send an Email, in future the above functionality can be extended to include attachments in the Email. For example, Reviewer can send an image/screenshot for a failure they have come across to the Author.

2. New functionality to send messages within expertiza

==Team Members==

Ashritha Bommagani (abommag@ncsu.edu)

Abhishek Arvindkumar Upadhyay (aaupadhy@ncsu.edu)

Harini Bharata (hbharat@ncsu.edu)

Sri Pallavi Damuluri (sdamulu@ncsu.edu)

File:Author email.jpeg

2022-05-02T02:48:49Z

Abommag:

File:Reviewer email.jpeg

2022-05-02T02:48:34Z

Abommag:

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-05-02T02:37:51Z

Abommag: /* Manual Testing */

==Introduction==

=== Problem Statement ===

This project adds a new feature which will enable a reviewer to send an Email to authors from inside Expertiza or for the author to email all those reviewing them. Often when reviewing another team’s submissions, a reviewer might notice that their deployment link is broken or there is some other issue. Currently, there is no way to email the authors. This features is proposed to solve this issue.

==Scope of the Feature==

- The feature allows authors to send a common Email to all the reviewers to report any issues or updates to their project to be reviewed. 
- It can allow a reviewer to send an email to a particular author if the reviewer wants to convey any issues found with that particular project. 
- Reviewers will be anonymous to authors. 

==Flowchart==

'''Author's View''' : The flow for authors to email reviewers regarding the work they have been reviewed for:

[[File:Author's view flowchart.jpg|1000px]] 

'''Reviewer's View''' : The flow for reviewers to email authors whose work they have reviewed:

[[File:Reviewers_View_Flowchart.jpg|1000px]] 

==UML Diagrams==

==='''Reviewers use case'''===

:: [[File:Reviewer_usecase.png|300px]] 

==='''Author's use case'''===

:: [[File:Author_usecase.png|300px]] 

==Proposed Changes==
===Code Implementation===

Expertiza code base already contains a module 'MailerHelper' which makes use of ActionMailer and contains methods to send emails to different users.
email_author_reviewers(subject, body, email) will be added which can be used in both the workflows to send an Email.
 
From the Reviewer's perspective, the following files will be updated and a new view will be created.

1. '''app/views/response/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/response_controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/response/author.html.erb :''' This is the new UI page. A mock up for the view is presented in the UI section. 
 

From Author's perspective, the following files will be updated and a new partial will be created to render the new view.

1. '''app/views/student_task/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/student_task controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/student_task/email_reviewers.html.erb :''' This is the new UI page. A mockup for the view is presented in the UI section. 

===UI===
As depicted below, Authors will be given a link on their main Assignment page “Send Email to Reviewers” which will take them to a new page where they are given a text box for typing and sending an email to all their reviewers. 

[[File:Assignment_page.png|800px]] 

[[File:Message-to-R.png|800px]] 

Reviewers will be given a similar text box link on their “Other’s Work” page next to each review that they are doing. 

[[File:Author_SS.png|800px]] 

[[File:Msg-to-A.png|800px]] 

==Implememtation==

The following are the screenshots of the changes that we made in the codebase.

===Send Email Method in MailerHelper===
We created a new method called send_mail_to_author_reviewers(subject, body, email), which accepts 3 parameters; subject, body and email. We used the same method for both sending emails to authors and reviewers.

[[File:Email_method.png|800px]] 

===Email from Reviewer to Author===
====View====
We modified the view to add a link that redirects the author to the form.

[[File:Email_the_author_link.png|1000px]] 

To create the author view, we created a new view called author.html.erb and created a form in that file, which asks the user to enter the subject and body.
[[File:Author_view.png|800px]] 

====Controller====
We created a method called send_email in the response_controller which handles the functionality for sending an email. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we pass the subject, body, and email from the view to the controller and send them to a method called send_mail_to_author_reviewers which is a part of MailerHelper. This method is responsible for sending emails to the author.

[[File:Send_email_author.png|1000px]] 

===Email from Author to Reviewers===
====View====
We modified the view to add a link that redirects the reviewer to the form.

[[File:Send_email_to_reviewers.png|1000px]] 

To create the reviewer view, we created a new view called email_reviewers.html.erb where we created a form that asks the user to enter the subject and body of the email.
[[File:Author_view.png|800px]] 

====Controller====
To send an email to all the reviewers, we created a method called send_email in the student_task_controller. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we go ahead and pass the parameters; subject, body, and reviewer.email to the send_mail_to_author_reviewers method from MailerHelper.

[[File:Send_email_reviewer.png|1000px]] 

==Testing==

===RSPEC Testing===

We tested the send_email method used in response_controller.rb and student_task_controller.rb which we have added to achieve the functionality of sending email to reviewers and authors. We have considered four scenarios which can occur while an email has to be sent, namely, user might not give the subject but the body, subject present but no body, both subject and body are present as desired in which case the email is sent on send email button click and the user is redirected to the student task list page and lastly, if there is no subject or body of the email is given, the user is flashed a message and is kept on the same page to fill in the desired fields. Below are the different test scenarios described above:

'''Test 1:'''
:: [[File:Test1.jpeg|800px]] 

'''Test 2:'''
:: [[File:Test2.jpeg|800px]] 

'''Test 3:'''
:: [[File:Test3.jpeg|800px]] 

'''Test 4:'''
:: [[File:Test4.jpeg|800px]] 

'''Test Result:''' We ran the test file using the command '''bundle exec rspec spec/controllers/response_controller_spec.rb''' and below is the test result.
:: [[File:Test result.jpeg|800px]] 

===Manual Testing===

For testing purposes, we are using the 'expertiza.debugging@gmail.com' as both author and reviewer.
There are two views that need to be tested manually.

1. Author View:

The author must follow the following steps:
1. Go to the Assignments page.
2. Click on the project for which they want to send email to the reviewers, in the student task view page then click on the right corner of the page where we have the link "send email to reviewers".
3. On clicking the above link, the author must be redirected to a page which will ask then for subject of email and text for the body of email.
4. Type the email subject, message and click on send.
5. Author can log in to the email to check if the message is received from receivers.

2. Reviewer View:

The reviewer must follow the following steps:
1. Go to the other's work page.
2. Next to every review that the reviewer is doing, will be a link on the right corner of the page called "send email to author".
3. On clicking the above link, the reviewer will be redirected to a page with text box for subject and message.
4. Type the email subject, message and click on send.
5. Log in to the email to check if the message is received.

==Future Scope==

1. Currently, only a subject and a text message are provided to send an Email, in future the above functionality can be extended to include attachments in the Email. For example, Reviewer can send an image/screenshot for a failure they have come across to the Author.

2. New functionality to send messages within expertiza

==Team Members==

Ashritha Bommagani (abommag@ncsu.edu)

Abhishek Arvindkumar Upadhyay (aaupadhy@ncsu.edu)

Harini Bharata (hbharat@ncsu.edu)

Sri Pallavi Damuluri (sdamulu@ncsu.edu)

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-05-01T23:41:58Z

Abommag: /* Testing */

==Introduction==

=== Problem Statement ===

This project adds a new feature which will enable a reviewer to send an Email to authors from inside Expertiza or for the author to email all those reviewing them. Often when reviewing another team’s submissions, a reviewer might notice that their deployment link is broken or there is some other issue. Currently, there is no way to email the authors. This features is proposed to solve this issue.

==Scope of the Feature==

- The feature allows authors to send a common Email to all the reviewers to report any issues or updates to their project to be reviewed. 
- It can allow a reviewer to send an email to a particular author if the reviewer wants to convey any issues found with that particular project. 
- Reviewers will be anonymous to authors. 

==Flowchart==

'''Author's View''' : The flow for authors to email reviewers regarding the work they have been reviewed for:

[[File:Author's view flowchart.jpg|1000px]] 

'''Reviewer's View''' : The flow for reviewers to email authors whose work they have reviewed:

[[File:Reviewers_View_Flowchart.jpg|1000px]] 

==UML Diagrams==

==='''Reviewers use case'''===

:: [[File:Reviewer_usecase.png|300px]] 

==='''Author's use case'''===

:: [[File:Author_usecase.png|300px]] 

==Proposed Changes==
===Code Implementation===

Expertiza code base already contains a module 'MailerHelper' which makes use of ActionMailer and contains methods to send emails to different users.
email_author_reviewers(subject, body, email) will be added which can be used in both the workflows to send an Email.
 
From the Reviewer's perspective, the following files will be updated and a new view will be created.

1. '''app/views/response/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/response_controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/response/author.html.erb :''' This is the new UI page. A mock up for the view is presented in the UI section. 
 

From Author's perspective, the following files will be updated and a new partial will be created to render the new view.

1. '''app/views/student_task/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/student_task controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/student_task/email_reviewers.html.erb :''' This is the new UI page. A mockup for the view is presented in the UI section. 

===UI===
As depicted below, Authors will be given a link on their main Assignment page “Send Email to Reviewers” which will take them to a new page where they are given a text box for typing and sending an email to all their reviewers. 

[[File:Assignment_page.png|800px]] 

[[File:Message-to-R.png|800px]] 

Reviewers will be given a similar text box link on their “Other’s Work” page next to each review that they are doing. 

[[File:Author_SS.png|800px]] 

[[File:Msg-to-A.png|800px]] 

==Implememtation==

The following are the screenshots of the changes that we made in the codebase.

===Send Email Method in MailerHelper===
We created a new method called send_mail_to_author_reviewers(subject, body, email), which accepts 3 parameters; subject, body and email. We used the same method for both sending emails to authors and reviewers.

[[File:Email_method.png|800px]] 

===Email from Reviewer to Author===
====View====
We modified the view to add a link that redirects the author to the form.

[[File:Email_the_author_link.png|1000px]] 

To create the author view, we created a new view called author.html.erb and created a form in that file, which asks the user to enter the subject and body.
[[File:Author_view.png|800px]] 

====Controller====
We created a method called send_email in the response_controller which handles the functionality for sending an email. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we pass the subject, body, and email from the view to the controller and send them to a method called send_mail_to_author_reviewers which is a part of MailerHelper. This method is responsible for sending emails to the author.

[[File:Send_email_author.png|1000px]] 

===Email from Author to Reviewers===
====View====
We modified the view to add a link that redirects the reviewer to the form.

[[File:Send_email_to_reviewers.png|1000px]] 

To create the reviewer view, we created a new view called email_reviewers.html.erb where we created a form that asks the user to enter the subject and body of the email.
[[File:Author_view.png|800px]] 

====Controller====
To send an email to all the reviewers, we created a method called send_email in the student_task_controller. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we go ahead and pass the parameters; subject, body, and reviewer.email to the send_mail_to_author_reviewers method from MailerHelper.

[[File:Send_email_reviewer.png|1000px]] 

==Testing==

===RSPEC Testing===

We tested the send_email method used in response_controller.rb and student_task_controller.rb which we have added to achieve the functionality of sending email to reviewers and authors. We have considered four scenarios which can occur while an email has to be sent, namely, user might not give the subject but the body, subject present but no body, both subject and body are present as desired in which case the email is sent on send email button click and the user is redirected to the student task list page and lastly, if there is no subject or body of the email is given, the user is flashed a message and is kept on the same page to fill in the desired fields. Below are the different test scenarios described above:

'''Test 1:'''
:: [[File:Test1.jpeg|800px]] 

'''Test 2:'''
:: [[File:Test2.jpeg|800px]] 

'''Test 3:'''
:: [[File:Test3.jpeg|800px]] 

'''Test 4:'''
:: [[File:Test4.jpeg|800px]] 

'''Test Result:''' We ran the test file using the command '''bundle exec rspec spec/controllers/response_controller_spec.rb''' and below is the test result.
:: [[File:Test result.jpeg|800px]] 

===Manual Testing===

For testing purposes, we are using the 'expertiza.debugging@gmail.com' as both author and reviewer.
There are two views that need to be tested manually.

1. Author View:

The author must follow the following steps:
1. Go to the Assignments page.
2. Click on the link which is placed on the right corner of the page called "send email to reviewers".
3. On clicking the above link, the author must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

2. Reviewer View:

The reviewer must follow the following steps:
1. Go to the other's work page.
2. Click on the link which is placed on the right corner of the page called "send email to author".
3. On clicking the above link, the reviewer must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

==Future Scope==

1. Currently, only a subject and a text message are provided to send an Email, in future the above functionality can be extended to include attachments in the Email. For example, Reviewer can send an image/screenshot for a failure they have come across to the Author.

2. New functionality to send messages within expertiza

==Team Members==

Ashritha Bommagani (abommag@ncsu.edu)

Abhishek Arvindkumar Upadhyay (aaupadhy@ncsu.edu)

Harini Bharata (hbharat@ncsu.edu)

Sri Pallavi Damuluri (sdamulu@ncsu.edu)

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-05-01T23:19:07Z

Abommag: /* RSPEC Testing */

==Introduction==

=== Problem Statement ===

This project adds a new feature which will enable a reviewer to send an Email to authors from inside Expertiza or for the author to email all those reviewing them. Often when reviewing another team’s submissions, a reviewer might notice that their deployment link is broken or there is some other issue. Currently, there is no way to email the authors. This features is proposed to solve this issue.

==Scope of the Feature==

- The feature allows authors to send a common Email to all the reviewers to report any issues or updates to their project to be reviewed. 
- It can allow a reviewer to send an email to a particular author if the reviewer wants to convey any issues found with that particular project. 
- Reviewers will be anonymous to authors. 

==Flowchart==

'''Author's View''' : The flow for authors to email reviewers regarding the work they have been reviewed for:

[[File:Author's view flowchart.jpg|1000px]] 

'''Reviewer's View''' : The flow for reviewers to email authors whose work they have reviewed:

[[File:Reviewers_View_Flowchart.jpg|1000px]] 

==UML Diagrams==

==='''Reviewers use case'''===

:: [[File:Reviewer_usecase.png|300px]] 

==='''Author's use case'''===

:: [[File:Author_usecase.png|300px]] 

==Proposed Changes==
===Code Implementation===

Expertiza code base already contains a module 'MailerHelper' which makes use of ActionMailer and contains methods to send emails to different users.
email_author_reviewers(subject, body, email) will be added which can be used in both the workflows to send an Email.
 
From the Reviewer's perspective, the following files will be updated and a new view will be created.

1. '''app/views/response/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/response_controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/response/author.html.erb :''' This is the new UI page. A mock up for the view is presented in the UI section. 
 

From Author's perspective, the following files will be updated and a new partial will be created to render the new view.

1. '''app/views/student_task/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/student_task controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/student_task/email_reviewers.html.erb :''' This is the new UI page. A mockup for the view is presented in the UI section. 

===UI===
As depicted below, Authors will be given a link on their main Assignment page “Send Email to Reviewers” which will take them to a new page where they are given a text box for typing and sending an email to all their reviewers. 

[[File:Assignment_page.png|800px]] 

[[File:Message-to-R.png|800px]] 

Reviewers will be given a similar text box link on their “Other’s Work” page next to each review that they are doing. 

[[File:Author_SS.png|800px]] 

[[File:Msg-to-A.png|800px]] 

==Implememtation==

The following are the screenshots of the changes that we made in the codebase.

===Send Email Method in MailerHelper===
We created a new method called send_mail_to_author_reviewers(subject, body, email), which accepts 3 parameters; subject, body and email. We used the same method for both sending emails to authors and reviewers.

[[File:Email_method.png|800px]] 

===Email from Reviewer to Author===
====View====
We modified the view to add a link that redirects the author to the form.

[[File:Email_the_author_link.png|1000px]] 

To create the author view, we created a new view called author.html.erb and created a form in that file, which asks the user to enter the subject and body.
[[File:Author_view.png|800px]] 

====Controller====
We created a method called send_email in the response_controller which handles the functionality for sending an email. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we pass the subject, body, and email from the view to the controller and send them to a method called send_mail_to_author_reviewers which is a part of MailerHelper. This method is responsible for sending emails to the author.

[[File:Send_email_author.png|1000px]] 

===Email from Author to Reviewers===
====View====
We modified the view to add a link that redirects the reviewer to the form.

[[File:Send_email_to_reviewers.png|1000px]] 

To create the reviewer view, we created a new view called email_reviewers.html.erb where we created a form that asks the user to enter the subject and body of the email.
[[File:Author_view.png|800px]] 

====Controller====
To send an email to all the reviewers, we created a method called send_email in the student_task_controller. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we go ahead and pass the parameters; subject, body, and reviewer.email to the send_mail_to_author_reviewers method from MailerHelper.

[[File:Send_email_reviewer.png|1000px]] 

==Testing==

===RSPEC Testing===

We tested the send_email method used in response_controller.rb and student_task_controller.rb which we have added to achieve the functionality of sending email to reviewers and authors. We have considered four scenarios which can occur while an email has to be sent, namely, user might not give the subject but the body, subject present but no body, both subject and body are present as desired in which case the email is sent on send email button click and the user is redirected to the student task list page and lastly, if there is no subject or body of the email is given, the user is flashed a message and is kept on the same page to fill in the desired fields. Below are the different test scenarios described above:

'''Test 1:'''
:: [[File:Test1.jpeg|800px]] 

'''Test 2:'''
:: [[File:Test2.jpeg|800px]] 

'''Test 3:'''
:: [[File:Test3.jpeg|800px]] 

'''Test 4:'''
:: [[File:Test4.jpeg|800px]] 

'''Test Result:''' We ran the test file using the command '''bundle exec rspec spec/controllers/response_controller_spec.rb''' and below is the test result.
:: [[File:Test result.jpeg|800px]] 

===Manual Testing===

For testing purposes, we are using the 'expertiza.debugging@gmail.com' as both author and reviewer.
There are two views that need to be tested manually.

1. Author View:

The author must follow the following steps:
1. Go to the Assignments page.
2. Click on the link which is placed on the right corner of the page called "send email to reviewers".
3. On clicking the above link, the author must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

2. Reviewer View:

The reviewer must follow the following steps:
1. Go to the other's work page.
2. Click on the link which is placed on the right corner of the page called "send email to author".
3. On clicking the above link, the reviewer must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

We are planning to cover the following test cases:

1. Author View:

1. Upon Clicking on the link which is placed on the right corner of the page called "send email to reviewers",
the author must be redirected to email_reviewers_view.
2. Check if the recipients of the email are indeed the reviewers of the authors' work.

2. Reviewer View:

1. Upon Clicking on the link which is placed on the right corner of the page called "send email to author",
the reviewer must be redirected to email_authors_view.
2. Check if the recipients of the email are indeed the authors of the reviewed work.

We will also be checking the below two edge cases:

1. Author View:
1. Once the email is sent, the author should be redirected to the assignment page with the corresponding success
or failure message
2. Reviewer View:
2. Once the email is sent, the author should be redirected to the "others work" page with the corresponding success
or failure message

==Future Scope==

1. Currently, only a subject and a text message are provided to send an Email, in future the above functionality can be extended to include attachments in the Email. For example, Reviewer can send an image/screenshot for a failure they have come across to the Author.

2. New functionality to send messages within expertiza

==Team Members==

Ashritha Bommagani (abommag@ncsu.edu)

Abhishek Arvindkumar Upadhyay (aaupadhy@ncsu.edu)

Harini Bharata (hbharat@ncsu.edu)

Sri Pallavi Damuluri (sdamulu@ncsu.edu)

File:Test3.jpeg

2022-05-01T22:37:58Z

Abommag:

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-05-01T22:33:21Z

Abommag: /* RSPEC Testing */

==Introduction==

=== Problem Statement ===

This project adds a new feature which will enable a reviewer to send an Email to authors from inside Expertiza or for the author to email all those reviewing them. Often when reviewing another team’s submissions, a reviewer might notice that their deployment link is broken or there is some other issue. Currently, there is no way to email the authors. This features is proposed to solve this issue.

==Scope of the Feature==

- The feature allows authors to send a common Email to all the reviewers to report any issues or updates to their project to be reviewed. 
- It can allow a reviewer to send an email to a particular author if the reviewer wants to convey any issues found with that particular project. 
- Reviewers will be anonymous to authors. 

==Flowchart==

'''Author's View''' : The flow for authors to email reviewers regarding the work they have been reviewed for:

[[File:Author's view flowchart.jpg|1000px]] 

'''Reviewer's View''' : The flow for reviewers to email authors whose work they have reviewed:

[[File:Reviewers_View_Flowchart.jpg|1000px]] 

==UML Diagrams==

==='''Reviewers use case'''===

:: [[File:Reviewer_usecase.png|300px]] 

==='''Author's use case'''===

:: [[File:Author_usecase.png|300px]] 

==Proposed Changes==
===Code Implementation===

Expertiza code base already contains a module 'MailerHelper' which makes use of ActionMailer and contains methods to send emails to different users.
email_author_reviewers(subject, body, email) will be added which can be used in both the workflows to send an Email.
 
From the Reviewer's perspective, the following files will be updated and a new view will be created.

1. '''app/views/response/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/response_controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/response/author.html.erb :''' This is the new UI page. A mock up for the view is presented in the UI section. 
 

From Author's perspective, the following files will be updated and a new partial will be created to render the new view.

1. '''app/views/student_task/view.html.erb :''' link to the new view will be added to this file. 
2. '''app/controllers/student_task controller.rb :''' implementation of the action method for the send button will be added here. Methods from MailerHelper will be used here. 
3. '''app/views/student_task/email_reviewers.html.erb :''' This is the new UI page. A mockup for the view is presented in the UI section. 

===UI===
As depicted below, Authors will be given a link on their main Assignment page “Send Email to Reviewers” which will take them to a new page where they are given a text box for typing and sending an email to all their reviewers. 

[[File:Assignment_page.png|800px]] 

[[File:Message-to-R.png|800px]] 

Reviewers will be given a similar text box link on their “Other’s Work” page next to each review that they are doing. 

[[File:Author_SS.png|800px]] 

[[File:Msg-to-A.png|800px]] 

==Implememtation==

The following are the screenshots of the changes that we made in the codebase.

===Send Email Method in MailerHelper===
We created a new method called send_mail_to_author_reviewers(subject, body, email), which accepts 3 parameters; subject, body and email. We used the same method for both sending emails to authors and reviewers.

[[File:Email_method.png|800px]] 

===Email from Reviewer to Author===
====View====
We modified the view to add a link that redirects the author to the form.

[[File:Email_the_author_link.png|1000px]] 

To create the author view, we created a new view called author.html.erb and created a form in that file, which asks the user to enter the subject and body.
[[File:Author_view.png|800px]] 

====Controller====
We created a method called send_email in the response_controller which handles the functionality for sending an email. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we pass the subject, body, and email from the view to the controller and send them to a method called send_mail_to_author_reviewers which is a part of MailerHelper. This method is responsible for sending emails to the author.

[[File:Send_email_author.png|1000px]] 

===Email from Author to Reviewers===
====View====
We modified the view to add a link that redirects the reviewer to the form.

[[File:Send_email_to_reviewers.png|1000px]] 

To create the reviewer view, we created a new view called email_reviewers.html.erb where we created a form that asks the user to enter the subject and body of the email.
[[File:Author_view.png|800px]] 

====Controller====
To send an email to all the reviewers, we created a method called send_email in the student_task_controller. The method also contains validations that throw an error if the user is trying to submit the form without filling out the subject or body. If there are no validation errors, we go ahead and pass the parameters; subject, body, and reviewer.email to the send_mail_to_author_reviewers method from MailerHelper.

[[File:Send_email_reviewer.png|1000px]] 

==Testing==

===RSPEC Testing===

We tested the send_email method in response_controller.rb and student_task_controller.rb which we have added to achieve the functionality of sending
email to reviewers and authors.

'''Test 1:'''
:: [[File:Test1.jpeg|800px]] 

'''Test 2:'''
:: [[File:Test2.jpeg|800px]] 

'''Test 3:'''
:: [[File:Test3.jpeg|800px]] 

'''Test 4:'''
:: [[File:Test4.jpeg|800px]] 

'''Test Result:'''
:: [[File:Testresult.jpeg|800px]] 

===Manual Testing===

For testing purposes, we are using the 'expertiza.debugging@gmail.com' as both author and reviewer.
There are two views that need to be tested manually.

1. Author View:

The author must follow the following steps:
1. Go to the Assignments page.
2. Click on the link which is placed on the right corner of the page called "send email to reviewers".
3. On clicking the above link, the author must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

2. Reviewer View:

The reviewer must follow the following steps:
1. Go to the other's work page.
2. Click on the link which is placed on the right corner of the page called "send email to author".
3. On clicking the above link, the reviewer must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

We are planning to cover the following test cases:

1. Author View:

1. Upon Clicking on the link which is placed on the right corner of the page called "send email to reviewers",
the author must be redirected to email_reviewers_view.
2. Check if the recipients of the email are indeed the reviewers of the authors' work.

2. Reviewer View:

1. Upon Clicking on the link which is placed on the right corner of the page called "send email to author",
the reviewer must be redirected to email_authors_view.
2. Check if the recipients of the email are indeed the authors of the reviewed work.

We will also be checking the below two edge cases:

1. Author View:
1. Once the email is sent, the author should be redirected to the assignment page with the corresponding success
or failure message
2. Reviewer View:
2. Once the email is sent, the author should be redirected to the "others work" page with the corresponding success
or failure message

==Future Scope==

1. Currently, only a subject and a text message are provided to send an Email, in future the above functionality can be extended to include attachments in the Email. For example, Reviewer can send an image/screenshot for a failure they have come across to the Author.

2. New functionality to send messages within expertiza

==Team Members==

Ashritha Bommagani (abommag@ncsu.edu)

Abhishek Arvindkumar Upadhyay (aaupadhy@ncsu.edu)

Harini Bharata (hbharat@ncsu.edu)

Sri Pallavi Damuluri (sdamulu@ncsu.edu)

File:Test result.jpeg

2022-05-01T22:16:17Z

Abommag:

File:Test4.jpeg

2022-05-01T22:16:02Z

Abommag:

File:Test2.jpeg

2022-05-01T22:15:33Z

Abommag:

File:Test1.jpeg

2022-05-01T22:12:51Z

Abommag:

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-04-11T21:38:46Z

Abommag:

File:Author usecase.png

2022-04-11T21:36:32Z

Abommag:

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-04-11T21:35:09Z

Abommag: /* Code Implementation */

File:Reviewer usecase.png

2022-04-11T21:32:57Z

Abommag: Abommag uploaded a new version of File:Reviewer usecase.png

File:Reviewer usecase.png

2022-04-11T21:24:53Z

Abommag:

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-04-05T23:33:14Z

Abommag:

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-04-05T23:30:26Z

Abommag: /* Testing */

CSC/ECE 517 Spring 2022 - E2246: Email your authors/reviewers, with complete tests

2022-04-05T00:07:18Z

Abommag: /* Team Members */

==Introduction==

===Expertiza Background ===
Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and grade the assignments submitted by students to Expertiza. Students can be assigned in teams based on their selection of the topics. It has functionalities such as peer reviews in which students can provide feedback on other's work which helps peer in better developing the project. It is supported by the National Science Foundation.

=== Problem Statement ===

This project adds a new feature which will enable a reviewer to send an Email to authors from inside Expertiza or for the author to email all those reviewing them. Often when reviewing another team’s submissions, a reviewer might notice that their deployment link is broken or there is some other issue. Currently, there is no way to email the authors. This features is proposed to solve this issue.

==Scope of the Feature==

- The feature allows authors to send a common Email to all the reviewers to report any issues or updates to their project to be reviewed. 
- It can allow a reviewer to send an email to a particular author if the reviewer wants to convey any issues found with that particular project. 
- Reviewers will be anonymous to authors. 

==Proposed Changes==

As depicted below, Authors will be given a link on their main Assignment page “Send Email to Reviewers” which will take them to a new page where they are given a text box for typing and sending an email to all their reviewers. 

[[File:Email-R.png|800px]] 

[[File:Message-to-R.png|800px]] 

Reviewers will be given a similar text box link on their “Other’s Work” page next to each review that they are doing. 

[[File:Email-to-A.png|800px]] 

[[File:Msg-to-A.png|800px]] 

===Flowchart===

'''Author's View''' : The flow for authors to email reviewers regarding the work they have been reviewed for:

[[File:Author's view flowchart.jpg|800px]] 

'''Reviewer's View''' : The flow for reviewers to email authors whose work they have reviewed:

[[File:Reviewers_View_Flowchart.jpg|800px]] 

==Testing==

Since this is a new Feature, Rspec test cases will be added for all the new methods introduced in the MailHelper class method.

===Manual Testing===

For testing purposes, we are using the 'expertiza.debugging@gmail.com' as both author and reviewer.
There are two views that need to be tested manually.

1. Author View:

The author must follow the following steps:
1. Go to the Assignments page.
2. Click on the link which is placed on the right corner of the page called "send email to reviewers".
3. On clicking the above link, the author must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

2. Reviewer View:

The reviewer must follow the following steps:
1. Go to the other's work page.
2. Click on the link which is placed on the right corner of the page called "send email to author".
3. On clicking the above link, the reviewer must be redirected to a page with an email text box.
4. Type the email message and click on send.
5. Log in to the email to check if the message is received.

==Design Pattern==

A design pattern is a general repeatable solution to a commonly occurring problem in software design. It is a description or template for how to solve a problem that can be used in many different situations.

We will be implementing the decorator design pattern in our code. The decorator pattern is used to add/extend existing functionality without modifying the existing code. Our code is also going to follow the open-closed principle.

== Team Members ==

Ashritha Bommagani (abommag@ncsu.edu)

Abhishek Arvindkumar Upadhyay (aaupadhy@ncsu.edu)

Harini Bharata (hbharat@ncsu.edu)

Sri Pallavi Damuluri (sdamulu@ncsu.edu)

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-28T04:55:55Z

Abommag:

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

'''submitted_content_controller''' had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

'''List of Issues in submitted_content_controller.rb'''

* Remove the comment “# hence use team count for the check”.

* Change the method name view to something more informative of the method.

* Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

* submit_file method is long. Try to break it into multiple methods.

* Change the method name folder_action to something like perform_folder_action.

* Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission, according to the ruby method naming conventions, as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
* The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

===Testing Plan===

This file (submitted_content_controller.rb) has very little coverage, so we have done the manual verification for making sure all the edge cases are taken care of after the refactoring is done. We have also run the following briefed rspec command to verify that our changes do not break any core functions.

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

Manual Testing:
[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

Edge Cases:
* Return error when the user who is not the assignment's participant trying to do the submission
* Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
* Return error when the URL/hyperlink is invalid
* Return error when the uploading file is smaller than 5 MB
* Return error when the uploading file is not pdf or file type is not among specified

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-28T04:45:07Z

Abommag: /* Problem Statement */

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

'''submitted_content_controller''' had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

'''List of Issues in submitted_content_controller.rb'''

* Remove the comment “# hence use team count for the check”.

* Change the method name view to something more informative of the method.

* Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

* submit_file method is long. Try to break it into multiple methods.

* Change the method name folder_action to something like perform_folder_action.

* Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission, according to the ruby method naming conventions, as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
* The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

===Testing Plan===

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

Manual Testing:
[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

Edge Cases:
* Return error when the user who is not the assignment's participant trying to do the submission
* Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
* Return error when the URL/hyperlink is invalid
* Return error when the uploading file is smaller than 5 MB
* Return error when the uploading file is not pdf or file type is not among specified

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-28T04:44:45Z

Abommag: /* List of Issues in submitted_content_controller.rb */

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

'''submitted_content_controller''' had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

::List of Issues in submitted_content_controller.rb

* Remove the comment “# hence use team count for the check”.

* Change the method name view to something more informative of the method.

* Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

* submit_file method is long. Try to break it into multiple methods.

* Change the method name folder_action to something like perform_folder_action.

* Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission, according to the ruby method naming conventions, as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
* The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

===Testing Plan===

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

Manual Testing:
[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

Edge Cases:
* Return error when the user who is not the assignment's participant trying to do the submission
* Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
* Return error when the URL/hyperlink is invalid
* Return error when the uploading file is smaller than 5 MB
* Return error when the uploading file is not pdf or file type is not among specified

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-28T04:43:07Z

Abommag: /* Problem Statement */

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

'''submitted_content_controller''' had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

===List of Issues in submitted_content_controller.rb===

* Remove the comment “# hence use team count for the check”.

* Change the method name view to something more informative of the method.

* Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

* submit_file method is long. Try to break it into multiple methods.

* Change the method name folder_action to something like perform_folder_action.

* Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission, according to the ruby method naming conventions, as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
* The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

===Testing Plan===

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

Manual Testing:
[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

Edge Cases:
* Return error when the user who is not the assignment's participant trying to do the submission
* Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
* Return error when the URL/hyperlink is invalid
* Return error when the uploading file is smaller than 5 MB
* Return error when the uploading file is not pdf or file type is not among specified

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-28T04:32:04Z

Abommag: /* Solutions devised */

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

'''submitted_content_controller''' had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

* Renaming methods to more appropriate and functionality specific names.
* some methods being too long which needed to be broken down.
* Remove or change the comment for the methods to be more meaningful and reflective on the actual functionality.

===List of Issues in submitted_content_controller.rb===

* Remove the comment “# hence use team count for the check”.

* Change the method name view to something more informative of the method.

* Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

* submit_file method is long. Try to break it into multiple methods.

* Change the method name folder_action to something like perform_folder_action.

* Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission, according to the ruby method naming conventions, as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
* The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

===Testing Plan===

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

Manual Testing:
[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

Edge Cases:
* Return error when the user who is not the assignment's participant trying to do the submission
* Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
* Return error when the URL/hyperlink is invalid
* Return error when the uploading file is smaller than 5 MB
* Return error when the uploading file is not pdf or file type is not among specified

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-28T01:29:49Z

Abommag:

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

'''submitted_content_controller''' had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

* Renaming methods to more appropriate and functionality specific names.
* some methods being too long which needed to be broken down.
* Remove or change the comment for the methods to be more meaningful and reflective on the actual functionality.

===List of Issues in submitted_content_controller.rb===

* Remove the comment “# hence use team count for the check”.

* Change the method name view to something more informative of the method.

* Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

* submit_file method is long. Try to break it into multiple methods.

* Change the method name folder_action to something like perform_folder_action.

* Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
* The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

===Testing Plan===

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

Manual Testing:
[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

Edge Cases:
* Return error when the user who is not the assignment's participant trying to do the submission
* Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
* Return error when the URL/hyperlink is invalid
* Return error when the uploading file is smaller than 5 MB
* Return error when the uploading file is not pdf or file type is not among specified

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-28T01:28:45Z

Abommag: /* List of Issues in submitted_content_controller.rb */

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

'''submitted_content_controller''' had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

* Renaming methods to more appropriate and functionality specific names.
* some methods being too long which needed to be broken down.
* Remove or change the comment for the methods to be more meaningful and reflective on the actual functionality.

===List of Issues in submitted_content_controller.rb===

* Remove the comment “# hence use team count for the check”.

* Change the method name view to something more informative of the method.

* Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

* submit_file method is long. Try to break it into multiple methods.

* Change the method name folder_action to something like perform_folder_action.

* Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
* The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

===Testing Plan===

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

Manual Testing:
[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

Edge Cases:
* Return error when the user who is not the assignment's participant trying to do the submission
* Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
* Return error when the URL/hyperlink is invalid
* Return error when the uploading file is smaller than 5 MB
* Return error when the uploading file is not pdf or file type is not among specified

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-21T23:59:28Z

Abommag: /* E2221. Refactoring of submitted_content_controller */

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

This controller has some problems that violate some essential Rails design principles. There are few methods in this controller that are far too long and need to be broken down into multiple methods. A few methods need to be commented as well. Few Methods need to be renamed.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Problems and solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
:: Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

* '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>
===Testing===
Please see the manual and rspec testing in the video below

Inorder to make sure the core functionalities of the Expertiza not break by our refactoring, we did rspec testing using the following command:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-21T23:56:56Z

Abommag: /* E2221. Refactoring of submitted_content_controller */

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

This controller has some problems that violate some essential Rails design principles. There are few methods in this controller that are far too long and need to be broken down into multiple methods. A few methods need to be commented as well. Few Methods need to be renamed.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Problems and solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: Too many nested if-else statements inside this method, can we do something better?

* '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

:: * '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>
===Testing===
Please see the manual and rspec testing in the video below

Inorder to make sure the core functionalities of the Expertiza not break by our refactoring, we did rspec testing using the following command:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-21T23:56:05Z

Abommag:

==E2221. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

This controller has some problems that violate some essential Rails design principles. There are few methods in this controller that are far too long and need to be broken down into multiple methods. A few methods need to be commented as well. Few Methods need to be renamed.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Problems and solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

:: Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

* '''Solution :''' The changes made for submit_file and reasons for them:

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

::Too many nested if-else statements inside this method, can we do something better?

:: * '''Solution :''' Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.
Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.
<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

:: * '''Solution :''' The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

::'''submitted_folder_controller.rb''' looks like below:

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end
</pre>
===Testing===
Please see the manual and rspec testing in the video below

Inorder to make sure the core functionalities of the Expertiza not break by our refactoring, we did rspec testing using the following command:
<pre>
rspec spec/controllers/submitted_content_controller_spec.rb
</pre>

[https://drive.google.com/file/d/1Y7edau8A0nOVaGah1wg3OcK5eGVcGLjh/view?usp=sharing '''Manual Testing Video with voice over''']

===References===

#[https://github.com/expertiza/expertiza Expertiza on GitHub]
#[https://github.com/LeooHsiang/expertiza/tree/beta Forked Repository Link]
#[http://expertiza.ncsu.edu/ Expertiza website]
#[https://expertiza.csc.ncsu.edu/index.php/Main_Page Expertiza Documentation]

CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

2022-03-20T21:53:03Z

Abommag: Created page with "==E1553. Refactoring of submitted_content_controller== This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source p..."

==E1553. Refactoring of submitted_content_controller==

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

__TOC__

===About Expertiza===

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

===Background on submitted_content_controller===

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

===Problem Statement===

This controller has some problems that violate some essential Rails design principles. There are few methods in this controller that are far too long and need to be broken down into multiple methods. A few methods need to be commented as well. Few Methods need to be renamed.

===Files modified and created in the project===

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

===Problems and solutions devised===

* '''Issue 1 :''' Remove the comment “# hence use team count for the check”.

:: Removed the comment on line 19 because the code no longer checks the team count.

* '''Issue 2 :''' Change the method name view to something more informative of the method.

:: The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

<pre>
def view
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Solution :''' The function name view is changed to prevent_submission as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

<pre>
def prevent_submission
@participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
return unless current_user_id?(@participant.user_id)

# @assignment is used to store the assignment of the participant
@assignment = @participant.assignment
# @can_submit is the flag indicating if the user can submit or not in current stage
@can_submit = false
@stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
redirect_to action: 'edit', id: params[:id], view: true
end
</pre>

* '''Issue 3 :''' Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

:: remove_hyperlink is used in the view

expertiza/app/views/submitted_content/_hyperlink.html.erb

Therefore, the comment is invalid. So we removed the comment on line 67.

* '''Issue 4 :''' submit_file method is long. Try to break it into multiple methods.

:: The submit_file method was defined as below:

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(participant.user_id)
file = params[:uploaded_file]
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', "/")
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(" ", '_') # safe_filename #curr_directory +
File.open(full_filename, "wb") {|f| f.write(file.read) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if get_file_type(safe_filename) == "zip"
end
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The file has been submitted.', request)
# Send link to update the review to all the reviewers assigned to this reviewee
mail_assigned_reviewers(team)
# participant.assignment.email(participant.id) rescue nil
if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Solution :''' The changes made for submit_file and reasons for them

:: '''def get_file_upload(participant, file, file_content) :''' The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

:: '''def notify_reviewers(participant):''' Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication.

<pre>
def submit_file
participant = AssignmentParticipant.find(params[:id])
# check if the current user id is same as participant user id
# Validate the user and on failure redirect to edit function for Submitting a file.
unless current_user_id?(participant.user_id)
flash[:error] = "Authentication Error"
redirect_to action: 'edit', id: participant.id
return
end

file = params[:uploaded_file]
file_size_limit = 5

# Check if file size is greater than the specified limit then redirect to edit function
unless check_content_size(file, file_size_limit)
flash[:error] = "File size must smaller than #{file_size_limit}MB"
redirect_to action: 'edit', id: participant.id
return
end

file_content = file.read

# check if file type is not among specified then redirect to edit function
unless check_content_type_integrity(file_content)
flash[:error] = 'File type error'
redirect_to action: 'edit', id: participant.id
return
end

# Submit the file successfully
full_filename = get_file_upload(participant, file, file_content)
assignment = Assignment.find(participant.parent_id)
team = participant.team
SubmissionRecord.create(team_id: team.id,
content: full_filename,
user: participant.name,
assignment_id: assignment.id,
operation: "Submit File")
ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
notify_reviewers(participant)
end
</pre>

<pre>
def get_file_upload(participant, file, file_content)
participant.team.set_student_directory_num
@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
curr_directory = if params[:origin] == 'review'
participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
else
participant.team.path.to_s + @current_folder.name
end
FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
safe_filename = file.original_filename.tr('\\', '/')
safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
File.open(full_filename, 'wb') { |f| f.write(file_content) }
if params['unzip']
SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
end
return full_filename
end
</pre>

<pre>
def notify_reviewers(participant)
participant.mail_assigned_reviewers

if params[:origin] == 'review'
redirect_to :back
else
redirect_to action: 'edit', id: participant.id
end
end
</pre>

* '''Issue 5 :''' Change the method name folder_action to something like perform_folder_action.

:: * '''Solution :''' Rename the folder_action method to perform_folder_action

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = '/'
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end
</pre>

* '''Issue 6 :''' We have commented the code where necessary to describe the function a method is performing for better understanding of code.

* '''Issue 7 :''' Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

:: * '''Solution :''' Created a new class and separated the methods perform_folder_action and all files related methods into this new controlled and named it submitted_folder_container.

<pre>
def perform_folder_action
@participant = AssignmentParticipant.find(params[:id])
return unless current_user_id?(@participant.user_id)

@current_folder = DisplayOption.new
@current_folder.name = "/"
@current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
if params[:faction][:delete]
delete_selected_files
elsif params[:faction][:rename]
rename_selected_file
elsif params[:faction][:move]
move_selected_file
elsif params[:faction][:copy]
copy_selected_file
elsif params[:faction][:create]
create_new_folder
end
redirect_to action: 'edit', id: @participant.id
end

# Raises error if there are issues with downloading of current selected folder and file in it
def download
begin
folder_name = params['current_folder']['name']
file_name = params['download']
raise "Folder_name is nil." if folder_name.nil?
raise "File_name is nil." if file_name.nil?
raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

send_file(folder_name + "/" + file_name, disposition: 'inline')
rescue StandardError => e
flash[:error] = e.message
end
end

def controller_locale
locale_for_student
end

# Moves file from current location/directory to specified directory/location and raises error if any problem occurs
def move_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:move]
begin
FileHelper.move_file(old_filename, newloc)
flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
rescue StandardError => e
flash[:error] = "There was a problem moving the file: " + e.message
end
end

# To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
# error is flashed.
def rename_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
begin
raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

File.send("rename", old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem renaming the file: " + e.message
end
end

# Function to delete selected file
def delete_selected_files
filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
FileUtils.rm_r(filename)
participant = Participant.find_by(id: params[:id])
assignment = participant.try(:assignment)
team = participant.try(:team)
SubmissionRecord.create(team_id: team.try(:id),
content: filename,
user: participant.try(:name),
assignment_id: assignment.try(:id),
operation: "Remove File")
ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
end

# Function to copy selected file
def copy_selected_file
old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
begin
raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
raise "The referenced file does not exist." unless File.exist?(old_filename)

FileUtils.cp_r(old_filename, new_filename)
rescue StandardError => e
flash[:error] = "There was a problem copying the file: " + e.message
end
end

# Function to create new folder
def create_new_folder
newloc = @participant.dir_path
newloc += "/"
newloc += params[:faction][:create]
begin
FileHelper.create_directory_from_path(newloc)
flash[:note] = "The directory #{params[:faction][:create]} was created."
rescue StandardError => e
flash[:error] = e.message
end
end
end