dancerfly / django-brambling

Event website manager, specifically designed for dance weekends or other events with multiple simultaneous tracks of classes.
BSD 3-Clause "New" or "Revised" License
11 stars 3 forks source link

Changed liability waiver to use Template. #808

Closed chigby closed 8 years ago

chigby commented 8 years ago

This seems better thant string.format() because (1) we are only using basic substitutions and (2) we can make those substitutions more safely.

Fixed #807

Currently, this is backwards compatible with the existing waivers in the database that use {bracketed} syntax. However, we have to convert it to ${dollars bracket} or just $dollars syntax to use the Template class. That's kind of a shim, so in the future we might want to:

  1. Convert existing waivers in the DB to use $event and $organization.
  2. Change the help text for this field to reflect the above.

But that's not urgent.

melinath commented 8 years ago

Why not just .replace() {event} and {organization} and leave it at that?

chigby commented 8 years ago

That would also work. I guess the way I saw this proceeding was eventually removing the replace calls altogether and just using the Template format. Though I suppose if the only things we want to substitute are the org name and the event name it doesn't really matter what the implementation is? I kind of assumed the liability waiver being formatted as an extensible template was valuable to us, but maybe that's just how it was done.

Do you know if a lot of groups customize this field with the template placeholders? I could also see solving this by not doing any on-the-fly replacement at all and just letting the organizer type in what they want, with the default value remaining what it is now.

melinath commented 8 years ago

The default right now is to have the template placeholders, but I don't know how much people rely on them. I'd be fine (long-term) with not doing any replacement at all – at which point we'd probably want to start raising ValidationErrors if we detect either string in there, or something.

chigby commented 8 years ago

@melinath fixed flake8 check :)