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

WIP - Allow inference of booleans in enums #1550

Closed
wants to merge 128 commits into from

Conversation

rossjones
Copy link
Contributor

@rossjones rossjones commented Jun 18, 2018

Where a field is a BooleanField or a NullBooleanField inference should
generate a lookup list with [Yes, No] and [Yes, No, Unknown] as
options (for radio buttons) respectively.

This will require changes to templates/radio.html so that Unknown does
not use Unknown as a value, instead leaving the value empty so that we
know the intended value is None.

This would be easier if lookuplists were tuples (like choices in Django)
so I'm considering implementing support for lists of tuples (as well as
the current list of strings) for lookuplists - the alternative is too
much logic in the template and special-casing the string 'Unknown'.
Does this make sense @fredkingham or should I just implement a helper
for converting Unknown -> "" in this specific case?

TODO

  • More tests
  • Ensure value is "" for Unknown
  • Check for i18n

Will fix #1540

davidmiller and others added 11 commits March 12, 2017 18:20
Where a field is a BooleanField or a NullBooleanField inference should
generate a lookup list with `[Yes, No]` and `[Yes, No, Unknown]` as
options (for radio buttons).

This will require changes to templates/radio.html so that Unkown does
not use Unknown as a value, instead leaving the value empty so that we
know the intended value is None.

This would be easier if lookuplists were tuples (like choices in Django)
so I'm considering implementing support for lists of tuples (as well as
the current list of strings) for lookuplists - the alternative is too
much logic in the template and special-casing the string 'Unknown'

Will fix #1540
@fredkingham
Copy link
Contributor

I think that's a good call.

It means we'll need changes to do changes to search but maybe supporting choices like Django uses choices is the right thing to do.

@fredkingham
Copy link
Contributor

fredkingham commented Jun 19, 2018

The other side of this argument is that lookup lists are exactly that, a list.

So they won't have a different value, but I think that's ok.

@rossjones
Copy link
Contributor Author

I was thinking of supporting both options actually, so you can have ["", ""] but for some cases (in particular this one) you can have [("",""), ("","")]

self.assertIn('No', rendered)
self.assertNotIn('Unknown', rendered)

def test_radio_infer_lookuplists_with_quantum_boolean(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best test name this week.

@@ -37,6 +37,7 @@ class HatWearer(models.EpisodeSubrecord):
name = dmodels.CharField(max_length=200)
hats = dmodels.ManyToManyField(Hat, related_name="hat_wearers")
wearing_a_hat = dmodels.BooleanField(default=True)
suits_a_hat = dmodels.NullBooleanField()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should pivot to this application.

@davidmiller
Copy link
Member

ISWYM. It's quite a dirty hack to special case "Unknown" / None / etc ...

If we're implementing general 'accept tuples' then I reckon you'd want to:

  • automatically feed in both halves of fields that have Django choices (I still don't know why you want different values for django choices, but whatever)
  • implement for {% radio|radio_vertical|select %} (Others?)
  • docs for the fact that this is now a thing you can do - presumably the way to roll this on non choices fields is to implement model.get_field_enum() ?

And I think we have to update opal.models.SerialisableFields.build_schema_for_field_name()['enum'] accordingly ?

And presumably something in opal/core/search/templates/extract.html - though @fredkingham may be well placed to advise?

@fredkingham
Copy link
Contributor

@davidmiller I think that's a great call, stick the logic in get_field_enum, return a dictionary.

It will require special casing in extract.html and the two radio button templates.

Yeah that's should work quite elegantly.

@rossjones
Copy link
Contributor Author

For the sake of expedience, and not leaving this PR open for ever, I was investigating the non-elegant special-case the enum in radio.html and it happens that the mix of angular template and django template is pretty horrible if you need to do that sort of thing. Mostly because the django template runs first, and if the template is bound to an angular [[ ]] then that data isn't available to django :(

Sorry, just needed to vent a bit :)

@fredkingham
Copy link
Contributor

Let it all out!

yeah if you wanted to iterate over something a key values of a js object sometimes but an array other times that get tricky...

I think there's an argument to changing lookup lists to dictionaries but that's way beyond the scope.

Other option would be to change it so that it evaluates the label item (ie not the one on the ng-model) if item == True, item == False, item == None, then put in a label.

I'm not sure that's that nice either...

@davidmiller
Copy link
Member

davidmiller commented Jun 21, 2018

Two other options:

You can look at the field type pre save and convert the string "unknown" to null in opal/static/js/opal/services/fields_translater.js you're after FieldTranslator.jsToSubrecord

You can check the field type and the value "Unknown" combination in opal.models.UpdatesFromDict.Update_from_dict where we already do type specific work for deserialization of dates/times and coping with many to many fields.

Either of these allow you to continue as if this is any other field with an enum/lookuplist as far as the template is concerned.

A Django NullBooleanField will come across in the /api/v0.1/record/ serialization as type: "null_boolean"

davidmiller and others added 27 commits October 12, 2018 17:02
Fixup: Re-word text that was apparently copy and pasted from Wardrounds.
fixes some of the links in the tutorial and puts in a more up to date…
The relevant controller was removed in 0.8 and this URL/View/Template is not used by any known application.
Move Pathway and Search Javascripts into the karma_defauts.js
@davidmiller
Copy link
Member

Re-pointed at 0.13 so closing this one...

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

Successfully merging this pull request may close these issues.

None yet

4 participants