CSC/ECE 517 Fall 2021 - E2140. Create new late policy successfully and fix Bank link: Difference between revisions
No edit summary |
|||
(15 intermediate revisions by one other user not shown) | |||
Line 1: | Line 1: | ||
__TOC__ | |||
==E2140. Create new late policy successfully and fix "Back" link== | |||
This page provides a description of the Expertiza based OSS project, E1240. | |||
===About Expertiza=== | ===About Expertiza=== | ||
Line 15: | Line 15: | ||
Create new late policy successfull and fix "Back" link. | Create new late policy successfull and fix "Back" link. | ||
'''Please note: As the initial bug fix was only a few lines of code/it was halfway fixed, after discussing with mentor, the project was converted into a "testing project", where we focused on writing tests for the implemented bugfixes. Hence, it hasn't been deployed and videos of the "tests"/"flow" have been recorded and uploaded to the attached link in the submission folder.''' | |||
==Fixes performed | ==Fixes performed== | ||
* Fix back link on "New late policy" page. | * Fix back link on "New late policy" page. | ||
* Fix incorrect flash message on the screen when new late policy is created | * Fix incorrect flash message on the screen when new late policy is created | ||
* Add controller unit tests for create actions for late_policies_controller | * Add controller unit tests for create and edit actions for late_policies_controller | ||
* Add functional test for late policy creation | * Add functional test for late policy creation | ||
* Add functional test for back button on new late policy page | * Add functional test for back button on new late policy page | ||
* Improve flow of late_policies_controller#edit action for better tests | * Improve flow of late_policies_controller#edit action for better tests | ||
* Fix late_policies_controller#edit action when it displayed two contradictory flashes | * Fix late_policies_controller#edit action when it displayed two contradictory flashes | ||
* | * SQL stack trace showed up when long string is entered in policy name, this has been fixed to show a different error to user | ||
* View was checked after a controller test to ensure that the correct message is shown to the user. | * View was checked after a controller test to ensure that the correct message is shown to the user. | ||
* | * Database was checked after controller test to verify whether correct changes happened or didn't. | ||
* model was also checked to ensure the database is in a correct state | |||
===About Late Policies Controller=== | ===About Late Policies Controller=== | ||
Line 39: | Line 41: | ||
3. spec/factories/factories.rb<br/> | 3. spec/factories/factories.rb<br/> | ||
==Files added for tests | ==Files added for tests== | ||
1. spec/controllers/late_policies_controller_spec.rb <br/> | 1. spec/controllers/late_policies_controller_spec.rb <br/> | ||
2. spec/features/late_policy_creation_spec.rb <br/> | 2. spec/features/late_policy_creation_spec.rb <br/> | ||
3. spec/features/late_policy_back_button_spec.rb <br/> | |||
== | ==Files modified for tests== | ||
1. spec/factories/factories.rb<br /> | |||
2. app/controllers/late_policies_controller.rb | |||
===Current Implementation=== | ===Current Implementation=== | ||
Line 54: | Line 59: | ||
=====Drawbacks and Solutions===== | =====Drawbacks and Solutions===== | ||
* '''Problem 1''': Back button was taking user to index of late_policies, and not assignment due dates page | * '''Problem 1''': Back button was taking user to index of late_policies, and not assignment due dates page | ||
::The back button was performing | ::The back button was performing its default action of going back to index page of late_policies, but it needs to send user to the assignment page. | ||
Before: <br /> | Before: <br /> | ||
Line 70: | Line 70: | ||
After: <br /> | After: <br /> | ||
<pre> | <pre> | ||
<%= link_to 'Back', | <%= link_to 'Back', edit_assignment_path(session[:edit_assignment]) %> | ||
</pre> | </pre> | ||
* '''Solution''': The link_to function is called with the | * '''Solution''': The link_to function is called with the link to the assignment which was last edited, so it will go to assginment page. | ||
* Use | * Use edit_assignment_path(session[:edit_assignment]) to go back to the previous page | ||
* '''Problem 2''': Creating new policy with blank fields does not give a proper error | * '''Problem 2''': Creating new policy with blank fields does not give a proper error | ||
::The existing code just shows a blank error, without specifying what has gone wrong. Hence, user will not know what is wrong when they created a new policy. | ::The existing code just shows a blank error, without specifying what has gone wrong. Hence, user will not know what is wrong when they created a new policy. | ||
* '''Solution''': The existing error handling code was changed to show the ActiveRecord::Invalid errors to user. And in case, a different exception occurs, a " | * '''Solution''': The existing error handling code was changed to show the ActiveRecord::Invalid errors to user. And in case, a different exception occurs, a "Something went wrong" message is shown, as this exception shouldn't be visible to users. | ||
Before: | Before: | ||
<pre> | <pre> | ||
rescue StandardError | rescue StandardError | ||
flash[:error] = "The following error occurred while saving the penalty policy: " | |||
</pre> | </pre> | ||
After: | After: | ||
Line 100: | Line 98: | ||
* '''Problem 3''': | * '''Problem 3''': Update `update` method of late_policies_controller | ||
:: | :: Existing update method was too convoluted to read and due to it's convolution, it was showing wrong errors on screen. | ||
* '''Solution''': The | * '''Solution''': | ||
The control flow of the `update` method was improved to use early exits when errors occur, instead of using booleans to manage execution. | |||
=== | ===Code improvements=== | ||
late_policies_controller#update | |||
Before | |||
<pre> | <pre> | ||
@penalty_policy = LatePolicy.find(params[:id]) | |||
flash[:error] = "The maximum penalty cannot be less than penalty per unit." if invalid_penalty_per_unit | |||
same_policy_name = false | |||
# if name has changed then only check for this | |||
if same_policy_name == LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id) | |||
flash[:error] = "The policy could not be updated because a policy with the same name already exists." | |||
if !same_policy_name && !invalid_penalty_per_unit | |||
begin | |||
@penalty_policy.update_attributes(late_policy_params) | |||
@penalty_policy.save! | |||
LatePolicy.update_calculated_penalty_objects(@penalty_policy) | |||
flash[:notice] = "The late policy was successfully updated." | |||
redirect_to action: 'index' | |||
rescue StandardError | |||
flash[:error] = "The following error occurred while updating the penalty policy: " | |||
redirect_to action: 'edit', id: params[:id] | |||
end | |||
elsif invalid_penalty_per_unit | |||
flash[:error] = "Cannot edit the policy. The maximum penalty cannot be less than penalty per unit." | |||
redirect_to action: 'edit', id: params[:id] | |||
elsif same_policy_name | |||
flash[:error] = "Cannot edit the policy. A policy with the same name " + params[:late_policy][:policy_name] + " already exists." | |||
redirect_to action: 'edit', id: params[:id] | |||
</pre> | </pre> | ||
After: | |||
<pre> | <pre> | ||
< | invalid_penalty_per_unit = params[:late_policy][:max_penalty].to_i < params[:late_policy][:penalty_per_unit].to_i | ||
# If penalty per unit is invalid, exit early | |||
if invalid_penalty_per_unit | |||
flash[:error] = "Cannot edit the policy. The maximum penalty cannot be less than penalty per unit." | |||
redirect_to action: 'edit', id: params[:id] and return | |||
end | |||
# Only do DB search if we pass the invalid penalty check | |||
@penalty_policy = LatePolicy.find(params[:id]) | |||
# if name has changed, then only check for this | |||
if params[:late_policy][:policy_name] != @penalty_policy.policy_name | |||
if LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id) | |||
flash[:error] = "Cannot edit the policy. A policy with the same name " + params[:late_policy][:policy_name] + " already exists." | |||
redirect_to action: 'edit', id: params[:id] and return | |||
end | |||
end | |||
begin | |||
@penalty_policy.update_attributes(late_policy_params) | |||
@penalty_policy.save! | |||
LatePolicy.update_calculated_penalty_objects(@penalty_policy) | |||
flash[:notice] = "The late policy was successfully updated." | |||
redirect_to action: 'index' and return | |||
rescue StandardError | |||
flash[:error] = "The following error occurred while updating the penalty policy: " | |||
redirect_to action: 'edit', id: params[:id] and return | |||
end | |||
<pre> | <pre> | ||
</pre> | </pre> | ||
=== | ===Test Plan=== | ||
Late Policies Controller's CRUD methods were tested via unit tests to test the REST API, and they were also tested via capybara to emulate a user using the website. | |||
Test cases considered: | |||
1. Submitting an empty form when creating a new late policy <br /> | |||
2. Using an maximum penalty that was less than the actual penalty <br /> | |||
3. Penalty unit entered as negative number <br /> | |||
4. Policy name is greater than 255 characters <br /> | |||
5. Prevent creation of policy name if a policy with the same name already exists <br /> | |||
6. Render the :new template successfully <br /> | |||
7. Render the :edit template successfully <br /> | |||
8. Prevents edit if policy name is being changed to an already existing policy's name <br /> | |||
For testing whether back button takes user back to the assginment edit page, we have used Capybara to emulate a user browsing the page. <br /> | |||
For capybara testing, we have considered the following tests: <br /> | |||
1. Filling the form with blank values and then expecting failures as well as database not being updated. <br /> | |||
2. Using a policy name that is longer than 255 characters (which is the database column length) <br /> | |||
3. If user enters a negative penalty per point, the policy should not be created <br /> | |||
4. Entering valid values in each field, should successfully create the late policy <br /> | |||
5. If penalty per unit is greater than max penalty , then creation of late policy should fail. <br /> | |||
===Automated Testing using RSPEC=== | |||
The current version of expertiza did not have any test for LatePoliciesController, and for back button. | |||
Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for LatePoliciesController, to test creation and expected failure to create scenarios. | |||
We have also added tests for back button on new late policy page. | |||
Similarly, we have added feature tests for late policy creation. | |||
Late policy creation tests with Capybara | |||
<pre> | <pre> | ||
$ rspec ./spec/features/late_policy_creation_spec.rb | |||
Randomized with seed 65056 | |||
..... | |||
Finished in 22.28 seconds (files took 9.71 seconds to load) | |||
5 examples, 0 failures | |||
Randomized with seed 65056 | |||
Coverage report generated for RSpec to /.../expertiza/coverage. 2074 / 15378 LOC (13.49%) covered. | |||
[Coveralls] Outside the Travis environment, not sending data. | |||
</pre> | </pre> | ||
: | REST API tests for LatePoliciesController: | ||
<pre> | <pre> | ||
$ rspec ./spec/controllers/late_policies_controller_spec.rb | |||
Randomized with seed 29277 | |||
........ | |||
Finished in 17.56 seconds (files took 8.64 seconds to load) | |||
8 examples, 0 failures | |||
Randomized with seed 29277 | |||
Coverage report generated for RSpec to /.../expertiza/coverage. 1587 / 15800 LOC (10.04%) covered. | |||
[Coveralls] Outside the Travis environment, not sending data. | |||
</pre> | </pre> | ||
Back button tests | |||
<pre> | <pre> | ||
$ rspec ./spec/features/late_policy_back_button_spec.rb | |||
Randomized with seed 15242 | |||
. | |||
Finished in 9.74 seconds (files took 8.95 seconds to load) | |||
1 example, 0 failures | |||
Randomized with seed 15242 | |||
Coverage report generated for RSpec to /.../expertiza/coverage. 2041 / 15378 LOC (13.27%) covered. | |||
</pre> | </pre> | ||
===References=== | ===References=== | ||
Line 145: | Line 250: | ||
#[https://github.com/vinay-deshmukh/expertiza/issues Issues made while development] | #[https://github.com/vinay-deshmukh/expertiza/issues Issues made while development] | ||
#[https://github.com/vinay-deshmukh/expertiza/pulls Pull requests made to the fork to document work] | #[https://github.com/vinay-deshmukh/expertiza/pulls Pull requests made to the fork to document work] | ||
#[https://github.com/vinay-deshmukh/expertiza/pulls?q=is%3Apr+is%3Aclosed Closed Pull Requests on Fork] |
Latest revision as of 00:03, 7 November 2021
E2140. Create new late policy successfully and fix "Back" link
This page provides a description of the Expertiza based OSS project, E1240.
About Expertiza
Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
Problem Statement
Create new late policy successfull and fix "Back" link.
Please note: As the initial bug fix was only a few lines of code/it was halfway fixed, after discussing with mentor, the project was converted into a "testing project", where we focused on writing tests for the implemented bugfixes. Hence, it hasn't been deployed and videos of the "tests"/"flow" have been recorded and uploaded to the attached link in the submission folder.
Fixes performed
- Fix back link on "New late policy" page.
- Fix incorrect flash message on the screen when new late policy is created
- Add controller unit tests for create and edit actions for late_policies_controller
- Add functional test for late policy creation
- Add functional test for back button on new late policy page
- Improve flow of late_policies_controller#edit action for better tests
- Fix late_policies_controller#edit action when it displayed two contradictory flashes
- SQL stack trace showed up when long string is entered in policy name, this has been fixed to show a different error to user
- View was checked after a controller test to ensure that the correct message is shown to the user.
- Database was checked after controller test to verify whether correct changes happened or didn't.
- model was also checked to ensure the database is in a correct state
About Late Policies Controller
This controller can be accessed by editing an assignment, going on "Due Dates" page and clicking on "New late policy".
Files modified in current project
1. app/controllers/late_policies_controller.rb
2. app/views/late_policies/new.html.erb
3. spec/factories/factories.rb
Files added for tests
1. spec/controllers/late_policies_controller_spec.rb
2. spec/features/late_policy_creation_spec.rb
3. spec/features/late_policy_back_button_spec.rb
Files modified for tests
1. spec/factories/factories.rb
2. app/controllers/late_policies_controller.rb
Current Implementation
- There were no tests present in beta branch, so tests for the CRUD operations were added.
- Edit action for late_policies_controller is complex to reason about, with booleans deciding whether there are errors.
- Back button does not lead to the assignment from which the new late policy page was created.
Drawbacks and Solutions
- Problem 1: Back button was taking user to index of late_policies, and not assignment due dates page
- The back button was performing its default action of going back to index page of late_policies, but it needs to send user to the assignment page.
Before:
<%= link_to 'Back', :action => 'index' %>
After:
<%= link_to 'Back', edit_assignment_path(session[:edit_assignment]) %>
- Solution: The link_to function is called with the link to the assignment which was last edited, so it will go to assginment page.
- Use edit_assignment_path(session[:edit_assignment]) to go back to the previous page
- Problem 2: Creating new policy with blank fields does not give a proper error
- The existing code just shows a blank error, without specifying what has gone wrong. Hence, user will not know what is wrong when they created a new policy.
- Solution: The existing error handling code was changed to show the ActiveRecord::Invalid errors to user. And in case, a different exception occurs, a "Something went wrong" message is shown, as this exception shouldn't be visible to users.
Before:
rescue StandardError flash[:error] = "The following error occurred while saving the penalty policy: "
After:
rescue ActiveRecord::RecordInvalid => exception flash[:error] = exception.record.errors.full_messages.join("<br />").html_safe redirect_to action: 'new' rescue StandardError => exception # In case it's some error, this error should not be shown to users flash[:error] = "Something went wrong"
- Problem 3: Update `update` method of late_policies_controller
- Existing update method was too convoluted to read and due to it's convolution, it was showing wrong errors on screen.
- Solution:
The control flow of the `update` method was improved to use early exits when errors occur, instead of using booleans to manage execution.
Code improvements
late_policies_controller#update Before
@penalty_policy = LatePolicy.find(params[:id]) flash[:error] = "The maximum penalty cannot be less than penalty per unit." if invalid_penalty_per_unit same_policy_name = false # if name has changed then only check for this if same_policy_name == LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id) flash[:error] = "The policy could not be updated because a policy with the same name already exists." if !same_policy_name && !invalid_penalty_per_unit begin @penalty_policy.update_attributes(late_policy_params) @penalty_policy.save! LatePolicy.update_calculated_penalty_objects(@penalty_policy) flash[:notice] = "The late policy was successfully updated." redirect_to action: 'index' rescue StandardError flash[:error] = "The following error occurred while updating the penalty policy: " redirect_to action: 'edit', id: params[:id] end elsif invalid_penalty_per_unit flash[:error] = "Cannot edit the policy. The maximum penalty cannot be less than penalty per unit." redirect_to action: 'edit', id: params[:id] elsif same_policy_name flash[:error] = "Cannot edit the policy. A policy with the same name " + params[:late_policy][:policy_name] + " already exists." redirect_to action: 'edit', id: params[:id]
After:
invalid_penalty_per_unit = params[:late_policy][:max_penalty].to_i < params[:late_policy][:penalty_per_unit].to_i # If penalty per unit is invalid, exit early if invalid_penalty_per_unit flash[:error] = "Cannot edit the policy. The maximum penalty cannot be less than penalty per unit." redirect_to action: 'edit', id: params[:id] and return end # Only do DB search if we pass the invalid penalty check @penalty_policy = LatePolicy.find(params[:id]) # if name has changed, then only check for this if params[:late_policy][:policy_name] != @penalty_policy.policy_name if LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id) flash[:error] = "Cannot edit the policy. A policy with the same name " + params[:late_policy][:policy_name] + " already exists." redirect_to action: 'edit', id: params[:id] and return end end begin @penalty_policy.update_attributes(late_policy_params) @penalty_policy.save! LatePolicy.update_calculated_penalty_objects(@penalty_policy) flash[:notice] = "The late policy was successfully updated." redirect_to action: 'index' and return rescue StandardError flash[:error] = "The following error occurred while updating the penalty policy: " redirect_to action: 'edit', id: params[:id] and return end <pre>
Test Plan
Late Policies Controller's CRUD methods were tested via unit tests to test the REST API, and they were also tested via capybara to emulate a user using the website.
Test cases considered:
1. Submitting an empty form when creating a new late policy
2. Using an maximum penalty that was less than the actual penalty
3. Penalty unit entered as negative number
4. Policy name is greater than 255 characters
5. Prevent creation of policy name if a policy with the same name already exists
6. Render the :new template successfully
7. Render the :edit template successfully
8. Prevents edit if policy name is being changed to an already existing policy's name
For testing whether back button takes user back to the assginment edit page, we have used Capybara to emulate a user browsing the page.
For capybara testing, we have considered the following tests:
1. Filling the form with blank values and then expecting failures as well as database not being updated.
2. Using a policy name that is longer than 255 characters (which is the database column length)
3. If user enters a negative penalty per point, the policy should not be created
4. Entering valid values in each field, should successfully create the late policy
5. If penalty per unit is greater than max penalty , then creation of late policy should fail.
Automated Testing using RSPEC
The current version of expertiza did not have any test for LatePoliciesController, and for back button. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for LatePoliciesController, to test creation and expected failure to create scenarios. We have also added tests for back button on new late policy page. Similarly, we have added feature tests for late policy creation.
Late policy creation tests with Capybara
$ rspec ./spec/features/late_policy_creation_spec.rb Randomized with seed 65056 ..... Finished in 22.28 seconds (files took 9.71 seconds to load) 5 examples, 0 failures Randomized with seed 65056 Coverage report generated for RSpec to /.../expertiza/coverage. 2074 / 15378 LOC (13.49%) covered. [Coveralls] Outside the Travis environment, not sending data.
REST API tests for LatePoliciesController:
$ rspec ./spec/controllers/late_policies_controller_spec.rb Randomized with seed 29277 ........ Finished in 17.56 seconds (files took 8.64 seconds to load) 8 examples, 0 failures Randomized with seed 29277 Coverage report generated for RSpec to /.../expertiza/coverage. 1587 / 15800 LOC (10.04%) covered. [Coveralls] Outside the Travis environment, not sending data.
Back button tests
$ rspec ./spec/features/late_policy_back_button_spec.rb Randomized with seed 15242 . Finished in 9.74 seconds (files took 8.95 seconds to load) 1 example, 0 failures Randomized with seed 15242 Coverage report generated for RSpec to /.../expertiza/coverage. 2041 / 15378 LOC (13.27%) covered.