Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

187263439 clean schools #43

Closed
wants to merge 5 commits into from
Closed

187263439 clean schools #43

wants to merge 5 commits into from

Conversation

ArushC
Copy link

@ArushC ArushC commented Mar 19, 2024

Pivotal Tracker Link

What this PR does:

Note: I am confident in these changes, so I merged them directly into the international schools branch. There is still an open PR from the international schools branch to the golden repo.

This PR fixes the Javascript that toggles the state "required" field when the country selected is/is not the USA. It also changes the frontend for the schools edit page so that the appropriate fields are now "required", and a JS popup appears that prevents you from submitting the form in the first place if the information that you are trying to submit is invalid. Earlier, it would let you submit the school if some info was invalid, but a flash message would appear. I completely removed this functionality and updated the tests accordingly.

This pull request fixes|implements (pick one...) ______.

Include screenshots, videos, etc.

Screen Shot 2024-03-18 at 5 51 28 PM Screen Shot 2024-03-18 at 5 51 37 PM Screen Shot 2024-03-18 at 5 52 11 PM

Who authored this PR?

@ArushC

How should this PR be tested?

  • Is there a deploy we can view?
    Yes:
  • What do the specs/features test?
    I added some additional test cases for invalid country and random state field for a non-USA country.
  • Are there edge cases to watch out for?
    nah

Are there any complications to deploying this?

Checklist:

  • Has this been deployed to a staging environment or reviewed by a customer?
  • Tag someone for code review (either a coach / team member)
  • I have renamed the branch to match PivotTracker's suggested one (necessary for BlueJay) (e.g. michael/12345-add-new-feature Any branch name will do as long as the story ID is there. You can use git checkout -b [new-branch-name])

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 69.94%. Comparing base (71b92b6) to head (53f0511).

Files Patch % Lines
app/controllers/schools_controller.rb 50.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #43       +/-   ##
===========================================
- Coverage   86.18%   69.94%   -16.25%     
===========================================
  Files          20       20               
  Lines         695      692        -3     
===========================================
- Hits          599      484      -115     
- Misses         96      208      +112     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ArushC
Copy link
Author

ArushC commented Mar 19, 2024

Ignore this PR for now. There is a heinous bug in the Cucumber tests that I cannot figure out.

@ArushC ArushC closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant