CLIxIndia-Dev / MiTiRobot

A chatbot for Telegram
MIT License
0 stars 1 forks source link

Form validation in the Admin tool, for the Element fields #1

Open coleshaw opened 7 years ago

coleshaw commented 7 years ago

In the README, it says that the Element text must start with start, for Telegram to parse things correctly. I would recommend actually enforcing this in the form, rather than rely on human authors / process. You can do this with a custom form validation. See references below:

https://docs.djangoproject.com/en/1.11/ref/contrib/admin/#admin-custom-validation https://docs.djangoproject.com/en/1.11/ref/forms/validation/ https://docs.djangoproject.com/en/dev/ref/validators/#writing-validators https://stackoverflow.com/questions/12608639/django-field-validation-in-model-and-in-admin https://stackoverflow.com/questions/24802244/custom-validation-in-django-admin

So something like value.startswith('start') would work?

And writing a test for that would be great...to verify the validator works as expected.

brandonhanks commented 7 years ago

So the start requirement is actually for only the root node in the response tree. This is because the first message that telegram allows new users to send to a bot is start. We tested using both a filter for when level=0 and also the MPTT package method is_root_node() to determine when to restart a conversation - it might be possible to combine that with a value test to come up with a way to do validation. Three other separate but related issues have come up as well that could be validated:

So a few further questions:

1) Should users be able to create multiple dialog trees (i.e. create more than one dialog root)? Previous discussion was that yes, they should be able to so that hidden menus, alternative conversations, or advanced features could be tested. So far only a single dialog tree has been created though, so changing the design (i.e. adding validation that a root node is start) is definitely possible.

2) It's not immediately clear how validation would affect the drag&drop interface provided by MPTT.

3) Should validation also cover the other three cases bulleted above? If one is being put in, it probably makes sense to do all of them together.

Reference to the model library: https://django-mptt.github.io/django-mptt/

coleshaw commented 7 years ago

Hm, so I'm very ignorant about how Telegram works, etc., so a couple of clarification questions about the above:

  1. Regarding the start node, do you mean that those advanced features require a root node that does not equal start? Is there a way to test for the "type" of feature that someone is adding, or is there a set of valid values for the root node (i.e. start, hidden, conversation, etc.)? If there is a known set, I would validate against those...sorry, I'm pretty ignorant about how Telegram works, so you'll have to help me here. Maybe it's what you mean by combining things via level or is_root_node().
  2. I think the Django admin validates on save, so I would imagine that you should be able to work that into the UI somehow? Looks like how to do this with MPTT is specified in the documentation.
  3. I think perhaps some combination of validation and testing should cover all the known use cases, plus additional corner cases. Yes, I think it would be better to cover the three cases above in form validation (and not just a 404, but all non-200 range / 301 / 302 URLs), but also include tests to make sure the code itself will handle the exceptions gracefully (in case someone does something that isn't considered in validation). When you say the bad URLs caused problems, are those problems seen by this Django app, or are they only seen by the end-users on their phones? (trying to understand if this code is able to handle these kinds of exceptions). Also are all the messages / URLs input by human users? That's a lot of potential bad input to catch...are there potentially other things that could break the message, besides the URLs? Like Unicode in keyboard button names, or invalid HTML code, or missing files, etc.?
coleshaw commented 7 years ago

One other thought -- I'm not clear on how the messages get shown to the end-users, but cleaning up user-input is always a pain. If you need to consider web-ish HTML security, might want to sanitize the message text with Django's strip_tag and escape methods. You might have to modify your URL-extraction code to be compatible. And add tests :-) But not sure if this is a concern / use case that you need to worry about...it's just that accepting random user input seems fraught with danger.