WIP: Add context to Timer model #575

[btoconnor]

Issue: #575

This adds a context field on the TImer class that allows 3rd party apps to store some information about the intent of the timer when it's created.

I also added code to pre-fill the Feeding form from the web UI if there was context stored on the Timer for a feeding, so that the drop downs will automatically select the values that were stored in the context. These drop downs are not locked, but just default values.

With this PR, I also had it validate the context coming in to make sure it made sense. While I can be convinced to do no validation, I'm not such a big fan of just having arbitrary dumping grounds on the db with a JSON field. Having some validation about what's going into the context to prevent a free-for-all makes sense to me so that there are some guard rails for what people put into context. Ie, if you're saying this timer is for a feeding, the attributes stored on the feeding should be valid feeding values.

NOTE: This is a "work in progress" PR because I'm having trouble running the test suite on my development environment (even on a clean master). I am running into the issues outlined in issue #483 - the temporary workaround there solved some of my issues, but not all. Running the tests I added manually pass, and the failures seem unrelated (to my eye, anyway) to my patch. However, I can't be 100% certain of that and I'd love some pointers on getting around these issues. Example of some of the issues I'm running into:

I see a lot of these in the start of each test run:

WARNING:root:Permission not found with name Can delete Head Circumference.
WARNING:root:Permission not found with name Can view Head Circumference.
WARNING:root:Permission not found with name Can add Height.
WARNING:root:Permission not found with name Can change Height.
WARNING:root:Permission not found with name Can delete Height.
WARNING:root:Permission not found with name Can view Height.
WARNING:root:Permission not found with name Can add Note.
WARNING:root:Permission not found with name Can change Note.
WARNING:root:Permission not found with name Can delete Note.
WARNING:root:Permission not found with name Can view Note.
WARNING:root:Permission not found with name Can add Pumping.
WARNING:root:Permission not found with name Can change Pumping.
WARNING:root:Permission not found with name Can delete Pumping.
WARNING:root:Permission not found with name Can view Pumping.

which might be related to the following:

======================================================================
FAIL: test_welcome (babybuddy.tests.tests_views.ViewsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/brian/Programming/babybuddy/babybuddy/tests/tests_views.py", line 82, in test_welcome
    self.assertEqual(page.status_code, 200)
AssertionError: 302 != 200

and others around:

======================================================================
FAIL: test_validate_unique_period (core.tests.tests_forms.ValidationsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/home/brian/Programming/babybuddy/core/tests/tests_forms.py", line 885, in test_validate_unique_period
    self.assertFormError(
  File "/home/brian/.local/share/virtualenvs/babybuddy-OAJSsbzD/lib/python3.8/site-packages/django/test/testcases.py", line 279, in patched_method
    return old_method(self, *args, **kwargs)
  File "/home/brian/.local/share/virtualenvs/babybuddy-OAJSsbzD/lib/python3.8/site-packages/django/test/testcases.py", line 199, in assertFormError
    self.assertFormError(context[form], field, errors, msg_prefix=msg_prefix)
  File "/home/brian/.local/share/virtualenvs/babybuddy-OAJSsbzD/lib/python3.8/site-packages/django/test/testcases.py", line 263, in patched_method
    return new_method(self, *args, **kwargs)
  File "/home/brian/.local/share/virtualenvs/babybuddy-OAJSsbzD/lib/python3.8/site-packages/django/test/testcases.py", line 736, in assertFormError
    self._assert_form_error(form, field, errors, msg_prefix, f"form {form!r}")
  File "/home/brian/.local/share/virtualenvs/babybuddy-OAJSsbzD/lib/python3.8/site-packages/django/test/testcases.py", line 709, in _assert_form_error
    self.assertEqual(field_errors, errors, msg_prefix + failure_message)
  File "/usr/lib/python3.8/unittest/case.py", line 912, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.8/unittest/case.py", line 905, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: [] != ['Another entry intersects the specified time period.'] : The non-field errors of form <TummyTimeForm bound=True, valid=False, fields=(child;start;end;milestone;tags)> don't match.

Any pointers to resolve this would be greatly appreciated!

[coveralls]

Coverage: 99.164% (-0.03%) from 99.194% when pulling 28628ce on btoconnor:btoconnor.add_timer_context into 1784696 on babybuddy:master.

[cdubz]

Well your tests passed in CI. There are some existing known inconsistencies re: datetime tests. See #483. Not sure about those permissions warnings though…

[cdubz]

Damn my review app flow failed 😭 will look at this soonish.

[cdubz]

Got a manually deployed instance of this PR now at http://pr-576.dokku.baby-buddy.net/ and will do an actual review in the next few days (for real this time...).

[cdubz]

Or now!

I also added code to pre-fill the Feeding form from the web UI if there was context stored on the Timer for a feeding, so that the drop downs will automatically select the values that were stored in the context. These drop downs are not locked, but just default values.

With this PR, I also had it validate the context coming in to make sure it made sense. While I can be convinced to do no validation, I'm not such a big fan of just having arbitrary dumping grounds on the db with a JSON field. Having some validation about what's going into the context to prevent a free-for-all makes sense to me so that there are some guard rails for what people put into context. Ie, if you're saying this timer is for a feeding, the attributes stored on the feeding should be valid feeding values.

I think for this initial pass we should not do these things. I like the idea and the model you're going for here with validation down to the field level but I'd rather we focus on getting this specifically for third party use. We can create a separate ticket to address how the context data might be handled by Baby Buddy itself.

E.g., one thing I can think is that we wouldn't necessarily want to limit the context data to just field values for the model. For that I'd rather we have some sort of structure defined (just in documentation, to be fair). I'm thinking something like a default_field_values at the top level of the JSON contextual data or something of that nature. I just want to make this as flexible as possible for any third-party integration that wants to use it.

[cdubz]

Per #576 (comment) -- let's remove the parts of this that look for structure in the app itself.

Aside from that the other thing we'll want to do on this PR is implement API support for management of the context field. It looks like DRF does support JSON field OOB so hopefully this will be an easy add: https://www.django-rest-framework.org/api-guide/fields/#jsonfield

[btoconnor]

I'm happy to make these changes (they seem reasonable) but I figured it would be helpful to outline the use-case here.

The way that we use the integration + Baby Buddy at this point in time[1] is that we tell Alexa to start the timer for feedings, and then we go and we finish the timer via the web app. So - we'd like to be able to say something like "Alexa, tell Baby Buddy to start a formula feeding", and then be able to finish the timer on the web app. Having bottle and formula pre-filled in the web UI would make this more of a natural interaction across platforms.

With the Alexa integration as it stands today, finishing the feeding with the ~4 Q/A dialog is pretty awkward and error-prone, and it's far more efficient to conclude the feeding via the Web UI.

That being said - I think it's reasonable to isolate the PR to just the data storage and chat about how cross platform works more in the future in a different PR.

1 - Hopefully we can make the Alexa integration more natural so that it's not so awkward to complete feedings via voice interaction, but it's not there right now. I would imagine anyone using any of the structured data via 3rd party integration would also expect to see the data they've already provided show up on the form as well, so it would be weird to have the data be localized to the 3rd party that is writing it, for now.

[cdubz]

OK, that's fair. We can try to work it out in this PR if it's really not that useful on it's own. I was just trying to keep the scope a little smaller here.

Do you have any thoughts on a structural guideline for the JSON data? Maybe we just reserve an _object property for the app that has a defined structure of:

{
  "model": "feeding",
  "fields": { "key": "value" }
}

What else?

[btoconnor]

I've been giving this some thought over a few days and what I keep coming back to is - I feel like there should be an agreed upon "core" namespace that when written, it should adhere to the same validations that the main app does - I keep thinking of literally the word core in the namespace - but maybe _babybuddy is good too. If any app (web UI, alexa app, whatever) puts something into that "reserved" context namespace, it'll get validated for its input.

Additionally - outside of that core namespace - the app is free to write whatever data it wants in its own namespace - so the Alexa app can write ad-hoc data in the "_babybuddy-alexa" namespace in the JSON payload.

This way, apps that want to store data that is relevant to the "general usability" of the timer (like what I outlined in my comment above) have the freedom to insert data into the context timer that will be reasonably guaranteed to be present when at least opening up the web ui (and other apps are free to read that data as they please, obviously), and will be validated all the same as if the user had inserted this data themselves at the end. This allows the web app to reasonably pre-fill data when concluding the timer because the data is validated on write.

I'm not convinced this is absolutely the right answer, but I like the flexibility it gives apps to both write data that is to be used cross-app, as well as a place to store app-specific data (if that arises).

As for the data that is to be stored in the core namespace - I don't have a ton of opinions. What I outlined in the PR is, well, obviously how I would do it at first glance., but I can be convinced to make it more general. That being said - there's only 3 types of timers (though they can obviously be used for anything), and right now only the feeding one has any real need for extra data, so I don't know that it's worth going into too much detail trying to generalize it a ton. Typically when I deal with this in other fields, I just through a version tag on the payload and reserve the right to move on from it in the future, with regular deprecations and stuff. But I totally understand if the prospect of that is even more headache-inducing, so I'm 100% open to implementing it in the general format you outlined.

[cdubz]

I think that makes sense and kinds of jives with what I was thinking anyway. Could you come up with some example payloads that illustrate your thinking? I'm not 100% sue I grasp it.

[btoconnor]

For sure - as the PR stands today the context looks like:

{
  "timer_type": "feeding",
  "method": "bottle",
  "type": "formula"
}

So with the changes outlined above it would be something like:

{
  "_babybuddy": {
    "timer_type": "feeding",
    "method": "bottle",
    "type": "formula"
  }
}

I think if we wanted to move to a more generic data specification - we'd have to figure out a way to specify the intended type for the timer - since that's a different model type. Though maybe just the presence of the model itself in the data - but you'd have to do some amount of work to make sure there's only 1, and that model is one of the 3 timer conclusion types anyway). I guess my point is - we would need to have code when writing the context in the app no matter what, so if we need that, the "general" data approach loses some of the appeal (at least in my eyes).

EDIT: I guess worth pointing out - as the PR stands today - it only validates each attribute you put into the context - not the context as a whole. If you only write "timer_type": "feeding" that would fine. If you wrote {"timer_type": "feeding", "type": "formula"} that would be okay too, even though the web form would usually default the "method" to "bottle".

In my eyes the context is mostly just to help pre-fill the data on the web ui - though obviously other apps might take it more authoritatively - ie, I imagine the Alexa app would not ask you to confirm those settings, it would just ask you for the data that isn't already in the context.

[cdubz]

I think if we wanted to move to a more generic data specification - we'd have to figure out a way to specify the intended type for the timer - since that's a different model type.

I think this is where I'm not making a connection -- how is the timer "type" different from the model?

I'm mostly interested in whether we can not deal with extra validation logic at all and just attempt to prefill form data generically based on a provided model and context data.

[btoconnor]

Yeah - you're correct - it would just be the model. Are you thinking of moving from

{
  "_babybuddy": {
    "timer_type": "feeding",
    "method": "bottle",
    "type": "formula"
  }
}

to something like

{
  "_babybuddy": {
    "model": "Feeding",
    "fields": {
      "type": "formula",
      "method": "bottle"
    }
  }
}

? If so, I think the updates to the PR to accommodate this would be fairly minimal.

[cdubz]

Yeah, I think so. The idea is that I'd like to ditch R79-R105 and refactor R65-R75 to attempt to get model fields based on _babybuddy.model and generically check the provided contextual data for matching names to model fields and attempt to pre-fill.

Maybe there are some potential complications here I'm missing?

[btoconnor]

As for the validation comment - is your concern around validating the context on write? ie - if someone released some form of bug to say the Alexa app, and the app wrote:

{
  "_babybuddy": {
    "model": "Feeding",
    "fields": {
      "type": "aliens",
      "method": "spaceship"
    }
  }
}

what would we want to happen?

My immediate thinking is that if you're an app writing data to the core _babybuddy namespace, the presumption should be that the data is valid for the intended model. If we just write it with no validation that those are acceptable values, it becomes harder for other apps to trust that data.

If I'm trying to utilize that data in the Alexa app to pre-fill fields, I don't want to be in a scenario where I have to check if any of these fields even make sense - I'd like to be able to count on the idea that if something is there, I can use it.

I can be convinced that we should discard the data if it's not valid rather than throwing some form of validation error, but storing it happily in the core namespace feels like it could quickly become an exercise in defensive programming, when we could just defend at ingestion.

[cdubz]

Yeah, I was thinking of discarding invalid data but I get what you're saying -- it would better to throw a validation error. I just want to be able to do that in a generic way... 🤔

[btoconnor]

Discarding the data wholesale if any part of the context is invalid seems fine to me, especially to start. If we can find a good way later on to preserve some of the context if some of the fields are invalid, I think that'd be great, but it feels to me like that can be an improvement later on.

[cdubz]

Ok, lets start there.

[billybonks]

Hey, all these features seem super helpful. I have been using Baby Buddy for a week and prototyping a new user interface, where it is essential to store data about the timer before the timer has ended.

The decision was to discard the data and not run the validation. @btoconnor, are you able to take this PR over the line?