Feat/device schema - #985
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -0,0 +1,35 @@ | |||
| { | |||
| "schemaType": "opensensemap.deviceSchema", | |||
| "schemaVersion": "1.0.0", | |||
There was a problem hiding this comment.
What's the difference between the schemaVersion and version? Is schemaVersion the foundation for futurel, different styles of schemas?
There was a problem hiding this comment.
Yes, schemaVersion would refer to the schema format, while version would refer to the specific device schema version
| if (newSensor.title && newSensor.unit && newSensor.sensorType) { | ||
| if ( | ||
| !deviceSchema && | ||
| !deviceSchemaVersionId && |
There was a problem hiding this comment.
Only add new sensor when no device schema is present? How about an early exit?
| setValue('deviceSchema', parsedSchema) | ||
| setValue('deviceSchemaVersionId', undefined) | ||
| setValue('deviceSchemaRegistrySelection', undefined) | ||
| setValue('selectedSensors', schemaSensors) |
There was a problem hiding this comment.
Why are you setting deviceSchemaVersionId to undefined in this case? Although we don't have selected a schema from a registry, it should still have a version, right?
Plus: Why set it to undefined in two different ways?
| setValue('deviceSchema', undefined) | ||
| setValue('deviceSchemaVersionId', undefined) | ||
| setValue('deviceSchemaRegistrySelection', undefined) | ||
| setValue('selectedSensors', []) |
There was a problem hiding this comment.
Why are you setting the same things to undefined in two different ways?
| setValue('deviceSchema', undefined) | ||
| setValue('deviceSchemaVersionId', schema.versionId) | ||
| setValue('deviceSchemaRegistrySelection', schema) | ||
| setValue('selectedSensors', schemaSensors) |
There was a problem hiding this comment.
See above.
(Maybe I just don't get the pattern here.)
| deviceSchema?.version ?? selectedRegistrySchema?.version | ||
| const selectedSchemaSensorCount = | ||
| deviceSchema?.sensors.length ?? | ||
| selectedRegistrySchema?.content.sensors.length |
There was a problem hiding this comment.
Your are using the selected registry schema as a fallback. Why? Shouldn't the schema information of the selected registry schema already be contained in the schema parameter?
| setValue('selectedSensors', []) | ||
| } | ||
|
|
||
| const useRegistrySchema = (schema: RegistryDeviceSchema) => { |
There was a problem hiding this comment.
Naming suggestion: applyRegistrySchema?
| setValue('selectedSensors', schemaSensors) | ||
| } | ||
|
|
||
| const hasLockedSchema = !!deviceSchema || !!deviceSchemaVersionId |
There was a problem hiding this comment.
hasLockedSchema - Isn't that more something like userHasSelectedSchema?
|
Should we add some explicit checks if the schema version as written in the json files ( |
| ALTER TABLE "device" ADD COLUMN "device_schema_id" text;--> statement-breakpoint | ||
| ALTER TABLE "device" ADD COLUMN "device_schema_name" text;--> statement-breakpoint | ||
| ALTER TABLE "device" ADD COLUMN "device_schema_version" text;--> statement-breakpoint | ||
| ALTER TABLE "device" ADD COLUMN "device_schema_hash" text;--> statement-breakpoint |
There was a problem hiding this comment.
This seems like a lot of redundant information. 🤔 How about having a separate table for the device schemas and then just link to a specific schema from a device?
| deviceSchemaId: text('device_schema_id'), | ||
| deviceSchemaName: text('device_schema_name'), | ||
| deviceSchemaVersion: text('device_schema_version'), | ||
| deviceSchemaHash: text('device_schema_hash'), |
There was a problem hiding this comment.
Why do we need to duplicate those entries? Aren't they already in the deviceSchemaVersion-table?
There was a problem hiding this comment.
Is this supposed to be a snapshot of the used deviceSchemaVersion during device creation time or something that should sync with any deviceSchemaVersion-changes?
If I understand the feature correctly, we can't update a deviceSchemaVersion once its created, right? 🤔
| return { | ||
| isUpdated: false, | ||
| message: | ||
| 'This device no longer matches its schema. Detach it from the schema before editing sensors.', |
There was a problem hiding this comment.
Can that ever be the case? Atm, I can't edit sensor for device schema devices.
| return { | ||
| isUpdated: false, | ||
| message: | ||
| 'Schema-backed sensors cannot be added or deleted. Detach the device from its schema first.', |
There was a problem hiding this comment.
See comment above: Can this case ever happen?
Or do we have those checks as a safeguard for the case when users edit a device via the api?
There was a problem hiding this comment.
Yes, although unlikely i would prefer to keep those checks just in case an edit occurs via API / the admin interface or something along those lines. Or do you think checks like these add too much noise to the file?









Type of Change
Implementation
Checklist
devbranchAdditional Information