-
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
SIMSBIOHUB-627: Fix Manage Telemetry Page #1452
Conversation
Resolves some warnings about incorrect html. Fix two incorrect useEffects for the device/deployment edit forms.
Openshift URLs for the PR Deployment: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## SIMSBIOHUB-627 #1452 +/- ##
==================================================
- Coverage 45.88% 45.74% -0.14%
==================================================
Files 891 881 -10
Lines 23336 23178 -158
Branches 3417 3410 -7
==================================================
- Hits 10707 10603 -104
+ Misses 12017 11965 -52
+ Partials 612 610 -2 ☔ View full report in Codecov by Sentry. |
@@ -5,7 +5,7 @@ import { getBctwUser } from '../../../services/bctw-service/bctw-service'; | |||
import { BctwTelemetryService } from '../../../services/bctw-service/bctw-telemetry-service'; | |||
import { getLogger } from '../../../utils/logger'; | |||
|
|||
const defaultLog = getLogger('paths/telemetry/manual'); | |||
const defaultLog = getLogger('paths/telemetry/vendor/deployments'); |
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.
Should this endpoint end with telemetry
? like vendor/deployments/telemetry? I would assume that fetching deployments would omit telemetry points and for the telemetry points to be a different request.
EDIT: It looks like this endpoint is making a request to BCTW and is never called in SIMS - can we remove it?
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.
Yeah this endpoint is going to be dropped. This change isn't really needed, I was just updating some other instances of 'telemetry/manual' and this got caught in the crossfire.
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.
Most of the related code will need to be updated or removed, etc, as part of the larger work needed to update the Manage Telemetry page.
I think this is due to the fact that the seed critter records in SIMS don't match real records in Critterbase. It looks like we do it in 2 places (in the app at least): If you add a new critter, then you can add them to a deployment, and they show up. (Creating the manual telemetry record fails, because its calling that old endpoint) |
Add placeholder when critterbase record cant be found.
Looks great! Made me think of something made small enough to add to this PR: we need to let users somehow remove vendor telemetry from a survey, in case they imported the wrong device or just want it gone. Right now you cannot remove vendor telemetry records and therefore can't get around the foreign key constraint preventing deployments from being deleted. |
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.
🥅
Quality Gate failedFailed conditions |
Links to Jira Tickets
https://apps.nrs.gov.bc.ca/int/jira/browse/SIMSBIOHUB-627
Description of Changes
Fix the add/edit/delete manual telemetry functionality.
Add server-side pagination/sorting to manage telemetry table.
Fix device credentials upload endpoint (remove call to bctw)
Misc
Minor UI cleanup/tweaks.
Resolves some warnings about incorrect html.
Fix two incorrect useEffects for the device/deployment edit forms.
Testing Notes