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

SIMSBIOHUB-648: Create Sampling Period Page #1446

Open
wants to merge 49 commits into
base: dev
Choose a base branch
from
Open

Conversation

mauberti-bc
Copy link
Collaborator

@mauberti-bc mauberti-bc commented Dec 5, 2024

Links to Jira Tickets

  • {Include a link to all applicable Jira tickets}

Description of Changes

The main goal of this PR is to separate the sampling site and sampling period CRUD operations. Previously sampling periods were created/updated as part of creating/updating a sampling site. Now, sampling periods have their own create/edit pages/components.

All of the changes are around that concept, handling all of the code that naturally broke as a result of the related database schema changes, and workflow changes, etc.

Testing Notes

New DB Schema

  • 1 new migration
    • See JSDoc notes in the migration.
  • 1 new procedure
    • Check constraint for the sample_period table.

Picture of the relevant tables, and how they look after running the migration:

New Functionality

  • Separate table/page for creating, editing, deleting sampling periods (via the Manage Sampling Information page).
    • New endpoints/services/repositories/interfaces

Updated Functionality

  • Updated the 3 sampling columns in the Observations table. Behaviour is similar to before, but underlying data is different.
  • Updated the sampling side bar (from the Manage Observations page). Behaviour is similar to before, but underlying data is different.
  • Updated Analytics tab (below the map on the Survey page). Behaviour is similar to before, but underlying data is different.
  • Updated the sampling information portion of the existing Observation CSV import backend code. The rest of the Observation CSV import code should work as before.

@mauberti-bc mauberti-bc added the Early Feedback Welcome PR is not finished, but early review feedback is welcomed label Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 52.71318% with 488 lines in your changes missing coverage. Please review.

Project coverage is 46.39%. Comparing base (7344aa4) to head (5a10cc9).

Files with missing lines Patch % Lines
...lds/AutocompleteSearch/AutocompleteSearchField.tsx 0.00% 61 Missing ⚠️
api/src/repositories/sample-period-repository.ts 27.84% 53 Missing and 4 partials ⚠️
api/src/services/observation-services/utils.ts 64.61% 39 Missing and 7 partials ⚠️
...formation/periods/SamplePeriodDataGridEditCell.tsx 2.32% 42 Missing ⚠️
api/src/repositories/analytics-repository.ts 41.93% 32 Missing and 4 partials ⚠️
api/src/repositories/technique-repository.ts 2.77% 35 Missing ⚠️
...g-information/sites/SampleSiteDataGridEditCell.tsx 0.00% 32 Missing ⚠️
...ts/data-grid/taxonomy/TaxonomyDataGridEditCell.tsx 0.00% 31 Missing ⚠️
...s/sample-site-repository/sample-site-repository.ts 76.47% 14 Missing and 6 partials ⚠️
...autocomplete/AsyncAutocompleteDataGridEditCell.tsx 0.00% 19 Missing ⚠️
... and 28 more
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1446      +/-   ##
==========================================
+ Coverage   46.07%   46.39%   +0.32%     
==========================================
  Files         910      913       +3     
  Lines       23796    24080     +284     
  Branches     3512     3604      +92     
==========================================
+ Hits        10963    11171     +208     
- Misses      12215    12275      +60     
- Partials      618      634      +16     

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

@mauberti-bc mauberti-bc added Ready For Review PR is ready for review and removed Early Feedback Welcome PR is not finished, but early review feedback is welcomed labels Dec 10, 2024
@mauberti-bc mauberti-bc marked this pull request as ready for review December 12, 2024 20:40
@NickPhura
Copy link
Collaborator

A few things I noticed:


When creating a period, if you select a site, and then remove the period from it, the validation is probably failing because the form won't submit, but there is no error message shown.

image


When editing an existing sample period, I can't select or change the sample sites.

image


Nit: I can save a period with a start and end date that are exactly the same (with no time).

image

If I pick the same times, then the save stops working, though there is no error message about why.


I have 1 sampling site, 1 technique, and 3 periods. The techniques aren't being combined in the observations table when adding a record:

image

@mauberti-bc
Copy link
Collaborator Author

  • I disallowed changing the site because at that point you may as well create a new period for the correct site and delete the wrong one. Seems along the lines of turning a skateboard into a car instead of just buying a car. Maybe worth allowing though?
  • We do want to allow a period that starts and stops on the same day with no time, in case the time is not known. I agree this seems off but is realistic data. (The duration column is null for those periods because there isn't an obvious way to represent the unknown duration)

@mauberti-bc
Copy link
Collaborator Author

image
There's a failing request when expanding a sampling site in the left side panel of the observations page. Otherwise haven't found any issues

Fix some existing req.query parsing logic.
@NickPhura NickPhura requested review from NickPhura and MacQSL January 13, 2025 21:21
Remove unused api param from delete observations endpoint.
Minor tweak to find endpoint filter type (allow them to be undefined as they were meant to be)
- Rename/refactor sample method migrations to come after the upgrade postgres migration.
@MacQSL
Copy link
Collaborator

MacQSL commented Jan 17, 2025

Heres some screenshots of some of the issues I found:

  1. When deleting the sampling site from the "Manage Observations" page it throws a failed to execute SQL error: column "survey_sample_site_id" does not exist
    image

  2. The sampling techniques table renders the wrong page when the rows overflow into the next page. (renders page 2 vs rendering page 1) when 11 sampling techniques http://localhost:7100/admin/projects/2/surveys/4/sampling
    image

  3. The survey page Sampling Information (Technique tab) says 11 records but renders nothing on page 2 http://localhost:7100/admin/projects/2/surveys/4/details
    image

- Fix techniques pagination (on both survey technique page, and manage sampling information page).
- Fix delete sample sites sql error.
- Update APIError -> HTTPError code.
@NickPhura
Copy link
Collaborator

Heres some screenshots of some of the issues I found:

Pushed up some changes that should address those 3 issues.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
8.9% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@@ -21,15 +22,15 @@ interface ICaptureStub {
* @returns {string | undefined}
*/
export const formatTimeString = (time?: string | null): string | undefined => {
const fullTime = dayjs(time, 'HH:mm:ss');
const fullTime = dayjs(time, DefaultTimeFormat);
const shortTime = dayjs(time, 'HH:mm');
Copy link
Collaborator

@MacQSL MacQSL Jan 20, 2025

Choose a reason for hiding this comment

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

nit: This can be updated to the new DefaultTimeFormatNoSeconds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants