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.

Sign up for GitHub

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

Jump to bottom

AP-4583: Extend upload evidence for partner #5929

Merged
merged 4 commits into from
Nov 6, 2023
Merged

AP-4583: Extend upload evidence for partner #5929

merged 4 commits into from
Nov 6, 2023

Conversation

skdejong
Copy link
Contributor

@skdejong skdejong commented Nov 1, 2023 •
edited
Loading

What

Extend upload evidence for partner

Add a new evidence type for when the client's partner needs to provide evidence of their employment as part of the Partner Means Assessment.

Checklist

Before you ask people to review this PR:

  • Tests and rubocop should be passing: bundle exec rake
  • Github should not be reporting conflicts; you should have recently run git rebase main.
  • There should be no unnecessary whitespace changes. These make diffs harder to read and conflicts more likely.
  • The PR description should say what you changed and why, with a link to the JIRA story.
  • You should have looked at the diff against main and ensured that nothing unexpected is included in your changes.
  • You should have checked that the commit messages say why the change was made.

@skdejong skdejong added ready for review Please review ruby Pull requests that update Ruby code labels Nov 1, 2023
@skdejong skdejong force-pushed the ap-4583-extend-upload-evidence-for-partner branch 8 times, most recently from a600d8d to 9ca3178 Compare November 2, 2023 08:57
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

RoseSAK
RoseSAK approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@RoseSAK RoseSAK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I had only a couple of comments/questions :) great work! 🚀 💯

config/locales/en/providers.yml
@@ -1580,7 +1581,7 @@ en:
multi_proceeding: Proceeding %{position} of %{total}
client_involvement_type:
show:
question: What is your client’s role in this proceeding?
question: What is your client's role in this proceeding?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of changes which look like they are just to do with the apostrophe - is this because these were previously backticks instead of apostrophes? Finding it a bit hard to tell just by looking!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was trying to fix some of the tests that were failing because of the ’ vs ' and I just ended up changing all of them rather than only the specific ones that I needed to make the specific failing tests pass.

db/schema.rb Outdated
add_foreign_key "linked_applications", "legal_aid_applications", column: "associated_application_id"
add_foreign_key "linked_applications", "legal_aid_applications", column: "lead_application_id"
add_foreign_key "linked_applications", "legal_aid_applications", column: "associated_application_id", validate: false
add_foreign_key "linked_applications", "legal_aid_applications", column: "lead_application_id", validate: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this has been added? This might be due to running migrations locally through rails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's what must have happened.

@RoseSAK RoseSAK added approved Approved by code reviewers and removed ready for review Please review labels Nov 3, 2023
@skdejong skdejong added the UAT label Nov 3, 2023
@skdejong skdejong force-pushed the ap-4583-extend-upload-evidence-for-partner branch from 9ca3178 to 9079d8d Compare November 3, 2023 12:14
@skdejong skdejong force-pushed the ap-4583-extend-upload-evidence-for-partner branch from 9079d8d to 463da39 Compare November 6, 2023 09:11
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@skdejong skdejong marked this pull request as ready for review November 6, 2023 10:24
@skdejong skdejong requested a review from a team as a code owner November 6, 2023 10:24
@skdejong skdejong added Ready to merge Has passed UAT testing and can be merged into master and removed UAT labels Nov 6, 2023
@skdejong skdejong merged commit 5bbabfa into main Nov 6, 2023
7 checks passed
@skdejong skdejong deleted the ap-4583-extend-upload-evidence-for-partner branch November 6, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RoseSAK RoseSAK RoseSAK approved these changes

Assignees

@colinbruce colinbruce

@skdejong skdejong

Labels
approved Approved by code reviewers Ready to merge Has passed UAT testing and can be merged into master ruby Pull requests that update Ruby code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skdejong @RoseSAK @colinbruce