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

API not capturing conversion_id property update #1011

Closed
tilersmyth opened this issue May 25, 2021 · 9 comments
Closed

API not capturing conversion_id property update #1011

tilersmyth opened this issue May 25, 2021 · 9 comments

Comments

@tilersmyth
Copy link

Describe the problem/bug

When converting an input measurement the 'conversion_id' prop of "device measurement settings" in the API stays null while the db reflects the update

Versions:

  • Mycodo Version: 8.10.1
  • Raspberry Pi Version: 3B+
  • Raspbian OS Version: Raspberry Pi OS Lite

Reproducibility

Convert an input measurement via UI
GET api/settings/device_measurements/by_device_id/[unique_id]
Bug: DB has conversion_id ref but API does not

thank you

@kizniche
Copy link
Owner

kizniche commented May 25, 2021

I'm not sure what's causing this. Without db.ForeignKey('conversion.unique_id'), it works. I'm not sure how to fix it while keeping the Foreign Key.

@tilersmyth
Copy link
Author

Interesting. I'm not as familiar with SQLAlchemy as I'd like to be, but I know other ORMs (such as TypeORM) require that you have a separate property if you want access to just the related table's id. Ex: "conversion": ForeignKey, "conversion_id": string..

Would you consider joining the conversion table for the api? I'd imagine that users would prefer having that data included if they've converted a measurement.

"device measurements": [
  {
  "channel": 0,
  "conversion": {
      "unique_id": "5b5e378a-42ac-4863-a2da-1e71897b244b",
      "unit": "F"
  },
  ...
  }
],

@kizniche
Copy link
Owner

That would be fine, I just don't know how to dot hat.

@tilersmyth
Copy link
Author

Not sure if it's something you're interested in implementing but I've accomplished what I described above by doing the following:

In ~/Mycodo/databases/measurements.py

  1. Add to DeviceMeasurements class:
    conversion = relationship("Conversion", foreign_keys="DeviceMeasurements.conversion_id")
  2. Add to DeviceMeasurementsSchema:
    conversion = Nested(ConversionSchema)

In ~/Mycodo/mycodo/mycodo_flask/api/sql_schema_fields.py

  1. Insert:
conversion_fields = api.model('Measurement Conversion Fields', {
    'id': fields.Integer,
    'unique_id': fields.String,
    ...
})
  1. Update the following in the device_measurement_fields api model:
    + 'conversion': fields.Nested(conversion_fields)
    - 'conversion_id': fields.String (Optional to remove, but currently always null)

In ~/Mycodo/mycodo/mycodo_flask/api/input.py

  1. in SettingsInputsUniqueID class add join to device_measurements query:
    change this:
list_measurements = return_list_of_dictionaries(
                measure_schema.dump(
                    DeviceMeasurements.query.filter_by(
                        device_id=unique_id).all(), many=True))

to this:

list_measurements = return_list_of_dictionaries(
                measure_schema.dump(
                    DeviceMeasurements.query.filter_by(
                        device_id=unique_id).join(DeviceMeasurements.conversion).all(), many=True))

Now the API returns the nested field if not null:
Screen Shot 2021-10-24 at 12 04 12 PM

@kizniche
Copy link
Owner

I'm very interested. Thanks for figuring that out and sharing. I'll see about implementing it for all the foreign keys.

@kizniche
Copy link
Owner

conversion = Nested(ConversionSchema)

What is the import for this?

@tilersmyth
Copy link
Author

from marshmallow_sqlalchemy.fields import Nested

@tilersmyth
Copy link
Author

Hey - quick fix for this solution. Need to make the change below to ensure this is a LEFT JOIN or "device measurements" will return null if conversion_id is undefined

- device_id=unique_id).join(DeviceMeasurements.conversion).all(), many=True))
+ device_id=unique_id).join(DeviceMeasurements.conversion, isouter=True).all(), many=True))

I'll close this issue. Thanks for implementing.

kizniche added a commit that referenced this issue Oct 31, 2021
@kizniche
Copy link
Owner

kizniche commented Nov 1, 2021

Thanks for figuring this out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants