Cleanup schools bug fix #45
Conversation
… error occured: [error messages]'
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
- Coverage 86.40% 86.03% -0.37%
==========================================
Files 20 20
Lines 706 709 +3
==========================================
Hits 610 610
- Misses 96 99 +3 ☔ View full report in Codecov by Sentry. |
|
@perryzjc @razztech @JacksonXu33 @realmichaeltao someone please review this PR |
|
@ArushC Nice Job. I finished the review for this PR and pushed some commits to fix some of the issues. I also added a new test. There was one tricky thing my personal computer hard to verify. Take a look at my review for |
|
@perryzjc "Take a look at my review for render 'new' && return above" --> what does this mean? I don't see any review. |
|
wait that's so weird I literally don't see any review here... hmmm... ok well I'll take a look at that. UPDATE: @perryzjc it looks fine to me. Check the latest heroku deployment to test. I forced pushed from this branch: https://sp24-01-bjc-teachers-d0e972207d82.herokuapp.com/teachers |
| @@ -45,10 +45,11 @@ def update | |||
| @school = School.find(params[:id]) | |||
| @school.assign_attributes(school_params) | |||
| if @school.save | |||
| flash[:success] = "Update #{@school.name} successfully." | |||
| flash[:success] = "Updated #{@school.name} successfully." | |||
There was a problem hiding this comment.
From codecov: Added line #L48 was not covered by tests
| redirect_to school_path(@school) | ||
| else | ||
| render "edit", alert: "Failed to submit information :(" | ||
| flash[:alert] = "An error occurred: #{@school.errors.full_messages.join(', ')}" |
There was a problem hiding this comment.
Added lines #L51 - L52 were not covered by tests
| @@ -81,7 +81,7 @@ def create | |||
| TeacherMailer.teacher_form_submission(@teacher).deliver_now | |||
| redirect_to root_path | |||
| else | |||
| redirect_to new_teacher_path, alert: "An error occurred while trying to save. #{@teacher.errors.full_messages}" | |||
| redirect_to new_teacher_path, alert: "An error occurred: #{@teacher.errors.full_messages.join(', ')}" | |||
include the new tests
Pivotal Tracker Link
What this PR does:
This PR does three main things:
fixes the Javascript that toggles the state "required" field when the country selected is/is not the USA, and fixes backend validations on the Schools#new and Schools#create pages.
Adds two more backend tests for the international schools feature through the RSpec testing framework.
Refactors ALL flash error messages in the application to adopt a standard format: "An error occured: [LIST OF ERRORS THAT OCCURED IN HUMAN-READABLE FORMAT]". I updated the Cucumber/RSpec tests accordingly. The error outputs are now considerably more informative than they were earlier ("Failed to submit information :(" does not tell me why the error occured).
I merged this PR directly into the international schools branch, for which a pull request to the Golden Repo is still open at beautyjoy#295.
This pull request fixes|implements (pick one...) ______.
Include screenshots, videos, etc.
Who authored this PR?
@ArushC
How should this PR be tested?
View the latest Heroku deployment, and look at the RSpec/Cucumber tests.
Yes: https://sp24-01-bjc-teachers-d0e972207d82.herokuapp.com/
EDIT: the current Heroku deployment contains the languages feature, not this one. Review that PR before this one and then force push this branch to the main on Heroku to deploy this.
Are there any complications to deploying this?
None
Checklist:
michael/12345-add-new-featureAny branch name will do as long as the story ID is there. You can usegit checkout -b [new-branch-name])