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

SIMSBIOHUB-353/325: Survey form updates #1149

Merged
merged 13 commits into from
Oct 31, 2023
Merged

SIMSBIOHUB-353/325: Survey form updates #1149

merged 13 commits into from
Oct 31, 2023

Conversation

al-rosenthal
Copy link
Contributor

@al-rosenthal al-rosenthal commented Oct 27, 2023 •
edited
Loading

Links to Jira Tickets

SIMSBIOHUB-325
SIMSBIOHUB-353

Description of Changes

  • Site selection strategies is now required for surveys
  • Removed field method from surveys

Testing Notes

  • You should be able to create and edit new and existing surveys without issue

@github-actions
Copy link

Openshift URLs for the PR Deployment:

@al-rosenthal al-rosenthal changed the base branch from dev to dev-staging October 27, 2023 19:18
@codecov
Copy link

codecov bot commented Oct 27, 2023 •
edited
Loading

Codecov Report

Merging #1149 (821e948) into dev-staging (e5158d6) will decrease coverage by 0.20%.
Report is 2 commits behind head on dev-staging.
The diff coverage is 0.00%.

@@               Coverage Diff               @@
##           dev-staging    #1149      +/-   ##
===============================================
- Coverage        62.14%   61.94%   -0.20%     
===============================================
  Files              554      547       -7     
  Lines            16914    16564     -350     
  Branches          2593     2539      -54     
===============================================
- Hits             10511    10261     -250     
+ Misses            5696     5621      -75     
+ Partials           707      682      -25
Files Coverage Δ
api/src/models/survey-create.ts 75.92% <ø> (-0.44%) ⬇️
api/src/models/survey-update.ts 80.00% <ø> (-0.33%) ⬇️
api/src/models/survey-view.ts 84.84% <ø> (-0.23%) ⬇️
api/src/paths/codes.ts 100.00% <ø> (ø)
api/src/paths/project/{projectId}/survey/create.ts 96.15% <ø> (ø)
api/src/paths/project/{projectId}/survey/list.ts 96.66% <ø> (ø)
...hs/project/{projectId}/survey/{surveyId}/update.ts 96.29% <ø> (ø)
...roject/{projectId}/survey/{surveyId}/update/get.ts 84.37% <ø> (ø)
...aths/project/{projectId}/survey/{surveyId}/view.ts 95.83% <ø> (ø)
api/src/repositories/code-repository.ts 100.00% <ø> (ø)
... and 14 more

... and 49 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@NickPhura NickPhura added the Staging Will merge into staging branch label Oct 27, 2023
curtisupshall
curtisupshall reviewed Oct 27, 2023
View reviewed changes
api/src/paths/project/{projectId}/survey/create.ts Outdated
@@ -202,7 +202,8 @@ POST.apiDoc = {
type: 'string'
},
field_method_id: {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably in our best interest to remove and field mthod code entirely, and not just allow it to be null. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree, I think it could cause some confusing leaving this in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, removed field method from the open api specs

Copy link
Contributor

Choose a reason for hiding this comment

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

good call, removed field method from the open api specs

What I mean is, we should probably remove any and all mention of field_method from the codebase. Example:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have since removed field method from the project 😅

@al-rosenthal al-rosenthal added the Ready For Review PR is ready for review label Oct 30, 2023
JeremyQuartech
JeremyQuartech reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@JeremyQuartech JeremyQuartech left a comment

Choose a reason for hiding this comment

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

It might make sense to update the db seed file, adding a value for site selection strategy, seeing as it should be required, and removing the value for field method id. A deprecation comment in the db column for field_method_id could be useful too.

@al-rosenthal
Copy link
Contributor Author

It might make sense to update the db seed file, adding a value for site selection strategy, seeing as it should be required, and removing the value for field method id. A deprecation comment in the db column for field_method_id could be useful too.

Good idea. I've removed the field method from the seed file and added the site strategy to the survey seed process

JeremyQuartech
JeremyQuartech approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@JeremyQuartech JeremyQuartech left a comment

Choose a reason for hiding this comment

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

Tested locally, everything looks good to me 👍

@NickPhura NickPhura marked this pull request as draft October 31, 2023 16:37
@sonarqubecloud SonarQubeCloud
Copy link

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
27.8% 27.8% Duplication

curtisupshall
curtisupshall approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@curtisupshall curtisupshall left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Tested this locally and everything works as expected. 👍

@al-rosenthal al-rosenthal marked this pull request as ready for review October 31, 2023 22:13
@github-actions GitHub Actions
Copy link

Openshift URLs for the PR Deployment:

@al-rosenthal al-rosenthal merged commit 7cb5142 into dev-staging Oct 31, 2023
31 of 33 checks passed
@al-rosenthal al-rosenthal deleted the SIMSBIOHUB-353 branch October 31, 2023 22:14
KjartanE pushed a commit that referenced this pull request Nov 6, 2023
@al-rosenthal @KjartanE
SIMSBIOHUB-353/325: Survey form updates (#1149)
244d535 
Removed field method and site selection strategies are now required on surveys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeremyQuartech JeremyQuartech JeremyQuartech approved these changes

@curtisupshall curtisupshall curtisupshall approved these changes

@NickPhura NickPhura Awaiting requested review from NickPhura

@KjartanE KjartanE Awaiting requested review from KjartanE

@GrahamS-Quartech GrahamS-Quartech Awaiting requested review from GrahamS-Quartech

Assignees
No one assigned
Labels
Ready For Review PR is ready for review Staging Will merge into staging branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@al-rosenthal @curtisupshall @JeremyQuartech @NickPhura