Refactor: remove kits

[timcowlishaw]

Work in progress on #241 - this removes kits and makes devices relate directly to sensors via components.

The migration is probably a good place to start to work out what's going on here at a high level.

Still TODO:

• See if we can remove OrphanDevices too

[timcowlishaw]

Fixed the following TODO items:

• Add logic to create components on a device when a reading is received for a new sensor.
• Decide what to do re: order of columns in the device CSV archive (the one currently failing test relates to this)
• Quitar spurious dates (recorded_at and added_at on device)
• Test RawStorer / Legacy Create (is this still needed?)

Regarding the dates - I've (for now) left the added_at field on the "/user/:id" and "/devices/world_map" endpoints as, in the first case, "added_at" is probably a better name for this property on the user (they already have their own created_at date), and in the case of the world map, we need to check that the JS doesn't expect it to be there. It's removed elsewhere though.

[timcowlishaw]

Hey @oscgonfer - I think this is now ready for a really thorough test on staging, and then a merge! We'll go through it on monday afternoon - have a good weekend in the meantime!

[timcowlishaw]

Note - this will have to be release along with an update to fablabbcn/smartcitizen-web to remove references to the kits endpoint. We should check also that onboarding works against this branch, which i will do locally and report back

[timcowlishaw]

TODO: a bit more tidying up of dates, while we're here:

• last_recorded_at on Device should become last_reading_at everywhere
• User joined_at should return to just being created_at for consistency.

[oscgonfer]

Hi there! Will running the db:migrate seems like there is a small issue, I believe coming from db/migrate/20230704150532_refactor_kits.rb:109 :

110               INSERT INTO components
111               (device_id, sensor_id, key, created_at, updated_at)
112               VALUES (?, ?, ?, ?, ?)
113             """, [device_id, sensor_id, key, created_at, updated_at])

Raising:

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "components_pkey"
DETAIL:  Key (id)=(81) already exists.

[oscgonfer]

Also, after checking the new structure, I believe we should have an additional column on the components table:

Conceptually, it will represent the idea of location or sensor bus (type integer) which will defaulted as 1, but that could be any number. The idea behind this is to prepare the data model to have the possibility to have more than one distinguishable component on a device with the same sensor_id.

We will then have to write a parser on mqtt-task to be able to convert what the kit sends (no longer it would be just a sensor_id, but a combined value between sensor_id and sensor_location, whenever it has more than one sensor with the same sensor_id, but in different data buses. For now, we can default the sensor_location to 0 in case we don't receive anything, but that way we set the scene later on.

[timcowlishaw]

Hi there! Will running the db:migrate seems like there is a small issue, I believe coming from db/migrate/20230704150532_refactor_kits.rb:109 :

110               INSERT INTO components
111               (device_id, sensor_id, key, created_at, updated_at)
112               VALUES (?, ?, ?, ?, ?)
113             """, [device_id, sensor_id, key, created_at, updated_at])

Raising:

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "components_pkey"
DETAIL:  Key (id)=(81) already exists.

Hey Oscar!

Aha, this is because of the primary key sequences getting out of sync when we restore from a backup, you'll need to run

ActiveRecord::Base.connection.tables.each do |t|
  ActiveRecord::Base.connection.reset_pk_sequence!(t)
end

in the rails console first (now we've deleted the weird devices, it shouldn't cause a problem).

This shouldn't need to be done in production, it's only because we're restoring from a backup that it's an issue.

[timcowlishaw]

(btw i've also pushed a small commit to get rid of the require "pry" statements that we didn't need and which were causing a seperate issue)

[oscgonfer]

Hi there! Will running the db:migrate seems like there is a small issue, I believe coming from db/migrate/20230704150532_refactor_kits.rb:109 :

110               INSERT INTO components
111               (device_id, sensor_id, key, created_at, updated_at)
112               VALUES (?, ?, ?, ?, ?)
113             """, [device_id, sensor_id, key, created_at, updated_at])

Raising:

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "components_pkey"
DETAIL:  Key (id)=(81) already exists.

Hey Oscar!

Aha, this is because of the primary key sequences getting out of sync when we restore from a backup, you'll need to run

ActiveRecord::Base.connection.tables.each do |t|
  ActiveRecord::Base.connection.reset_pk_sequence!(t)
end

in the rails console first (now we've deleted the weird devices, it shouldn't cause a problem).

This shouldn't need to be done in production, it's only because we're restoring from a backup that it's an issue.

Aha!! OK! that did it! Great work! 👏

Will test it a bit and come back to you. I will put it on a separate comment for traceability of TO-DOs

[oscgonfer]

Keeping track here of things @timcowlishaw

• Remove raw_value and prev_raw_value from sensor fields in device
  • Add timestamp of the actual measurement for in replacement of raw_value and prev_raw_value for latest reading
• Additional field in component as described here Refactor: remove kits #246 (comment)
• User joined_at should return to just being created_at for consistency (Refactor: remove kits #246 (comment))
• Add a way to populate the device.description based on hardware info... (this can be problematic, because the description can be changed in the device>edit view in the front-end, but let's talk about it)

[timcowlishaw]

Aha thanks @oscgonfer that's super helpful. I'll get all that sorted this week!

[timcowlishaw]

Hey @oscgonfer , I've just pushed a commit with the following changes:

• Add 'location' column to components.
• Remove user joined_at alias, revert to created_at for consistency
• Rename devices last_recorded_at column to last_reading_at, and remove the alias, for consistency.
• remove 'raw_value' and 'raw_prev_value' from the data exposed for devices.

I think it'd be good to have a chat about "adding the timestamp of the actual measurement" and "populating the device description" next week if poss - there's a few things there i'm not clear on!

[oscgonfer]

Hey @oscgonfer , I've just pushed a commit with the following changes:

* Add 'location' column to components.

* Remove user joined_at alias, revert to created_at for consistency

* Rename devices last_recorded_at column to last_reading_at, and remove the alias, for consistency.

* remove 'raw_value' and 'raw_prev_value' from the data exposed for devices.

I think it'd be good to have a chat about "adding the timestamp of the actual measurement" and "populating the device description" next week if poss - there's a few things there i'm not clear on!

Thanks @timcowlishaw ! Let's talk about these points in person.

Only one comment: we should think about the location name (and possible confusions that derive from that). Maybe we can give it a thought (bus... or something similar?)

[oscgonfer]

Regarding the timestamp, let's take two payloads of the same device:

{
    t:2023-10-04T09:39:55Z,
    10:99,
    14:256,
    55:16.12,
    56:65.84,
    53:47.51,
   58:102.08
}

{
    t:2023-10-04T09:40:55Z,
   10:99,
   14:256
}

The second payload has data for only two sensors. But on the device endpoint we have no way to know when the last actual data of a sensor took place, as it is all associated only with last_reading_at

[timcowlishaw]

Added migration to rename 'location' to 'bus' - now just pending the work on per-component last reading times, which i'll get to after the remaining bugfixes elsewhere!

[timcowlishaw]

I've also added per-component last_reading_at timestamps, updated with each new reading!

[timcowlishaw]

BTW @oscgonfer when this refactor is done and merged, I'd like to start thinking about refactoring and improving the test coverage for the data storers and MQTT handler - they're extremely "enmarañados" at the moment and with quite flaky coverage, which is making me very nervous about changing anything to do with them, and they're a pretty key part of the system. We can discuss next time we see each other!

[oscgonfer]

I've also added per-component last_reading_at timestamps, updated with each new reading!

I am so happy about this one!

❤️

[timcowlishaw]

Squashed and rebased onto master!

[timcowlishaw]

Hi @timcowlishaw

I was checking this, and I found out one potential issue with the default_keys.

We never made a separate issue for this, so just noting here that we fixed this with the change to the default_keys logic

[oscgonfer]

Hi @timcowlishaw
I was checking this, and I found out one potential issue with the default_keys.

We never made a separate issue for this, so just noting here that we fixed this with the change to the default_keys logic

Yes, sorry! Thank you!