-
Notifications
You must be signed in to change notification settings - Fork 9
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
Project edit pre-work setup #165
Conversation
A JIRA Issue ID is missing from your branch name! 🦄 Your branch: Edit_general_structure If this is your first time contributing to this repository - welcome! Please refer to jira-lint to get started. Without the JIRA Issue ID in your branch name you would lose out on automatic updates to JIRA via SCM; some GitHub status checks might fail. Valid sample branch names:‣ feature/shiny-new-feature--mojo-10' |
Codecov Report
@@ Coverage Diff @@
## dev #165 +/- ##
==========================================
- Coverage 48.26% 45.99% -2.28%
==========================================
Files 111 114 +3
Lines 2101 2207 +106
Branches 472 511 +39
==========================================
+ Hits 1014 1015 +1
- Misses 997 1102 +105
Partials 90 90
Continue to review full report at Codecov.
|
A JIRA Issue ID is missing from your branch name! 🦄 Your branch: Edit_general_structure If this is your first time contributing to this repository - welcome! Please refer to jira-lint to get started. Without the JIRA Issue ID in your branch name you would lose out on automatic updates to JIRA via SCM; some GitHub status checks might fail. Valid sample branch names:‣ feature/shiny-new-feature--mojo-10' |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
can we add tests to missing areas in models/
and queries/
files? should be mostly copies of existing tests in other places to ensure the classes set properties correctly in constructor/that the sqlstatement returned is not null when expected :)
* @export | ||
* @class PostProjectObject | ||
*/ | ||
export class PostProjectObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a test for this class? :)
* @export | ||
* @class PostPermitData | ||
*/ | ||
export class PostPermitData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test for this class as well
* @export | ||
* @class PostFundingData | ||
*/ | ||
export class PostFundingData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this class too
@shreyas, I think we might need to tackle the tests in another PR, so that we can start using this common code base BUT ... we should have a place to capture the list of tests to be done sooner than later |
hmmm... yeah I'm ok leaving it to another PR but 2.28% is quite a lot and definitely needs to be addressed |
Yeah, we can help address it better once we have it in dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good in general .. the codecov went down, but I think it's worth putting the PR through and we can collectively address the test coverage
@NickPhura, what approach should go with?
I'd rather get this merged now, and include the necessary tests in each of the individual edit ticket PRs. This is working when I test it with postman, but it hasn't been hooked up the the frontend, so it might be that I've overlooked something. It would be a bit safer to write tests for each edit workflow after getting its full end-to-end working. |
Yeah, we can/should add tests as we flush out each of the edit pieces. But right now, with this being an incomplete piece of the round-trip workflow, I'd prefer to hold off in case something critical needs to change. |
Overview
This PR contains the following changes
This PR contains the following types of changes
Checklist
A list of items that are good to consider when making any changes.
Note: this list is not exhaustive, and not all items are always applicable.
General
Code
Style
Documentation
Tests
Linting/Formatting
See the
lint
commands in package.jsonSee the
format
commands in package.jsonSonarCloud
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Screenshots
Please add any relevant UI screenshots, if applicable.