TabbycatDebate / tabbycat

Debating tournament tabulation software for British Parliamentary and a variety of two-team parliamentary formats
https://tabbycat.readthedocs.io/
GNU Affero General Public License v3.0
247 stars 841 forks source link

Make venue constraints in line with adjudicator conflicts #920

Open tienne-B opened 6 years ago

tienne-B commented 6 years ago

For consistency, venue constraints should be analogous to adjudicator conflicts, and thus be edited similarly. This would mean splitting the constraints table, getting rid of the generic foreign keys; and having many constraint views (Division constraints view linked to only if enabled).

What would be hard is the priority field, which could be made into a more user-friendly ~"High/Med/Low" dropdown (accessibility being high, etc.)~ boolean on VenueCategory. (changed my mind)

Working on this.

czlee commented 6 years ago

Hmm, yeah. There's actually an even bigger question here: when should we use generic foreign keys? I've changed my approach (and done associated refactoring!) several times in the past and I'm not confident that I've got all these decisions right. There are plenty of blog posts that Django developers have written on the topic, "should I use content types?" or "should I use generic relations?" and so on, but I'm not quite sure what principles I subscribe to. So I'd like to discuss your rationales for this before you go ahead and refactor it like this.

You're right to point out that the current code base isn't consistent in this, but you shouldn't assume that the odd one out is "wrong" or that any is "right"—it's just a reflection of my having sampled different approaches at different times.

tienne-B commented 6 years ago

While one is not wrong over the other, the benefit of having the multiple tables instead of the one with a generic key is the ease of use. As a user, I don't even want to hear about primary keys. However they are mandatory to create the constraints. The text-box selector does not work on some browsers (Safari) and that it is awkward, as well as the drop-down for the target type. That's why having the multiple pages is better, but there is also the benefit of consistency that comes with :)

I'd be one to simplify creating venue constraints. For one, there shouldn't be a priority setting for each constraint; it should rather be a (un-)important boolean on the venue category.

czlee commented 6 years ago

If the issue is the UI, remember that's not really intrinsic to the database schema. There's always in principle some way to make a good selector widget for any database schema you care to imagine. It's just a matter of whether someone else has written the widget, or whether you have to write it yourself. The selector we have there now is just what we put in place when getting it going; if we had more time to invest in making play nice, we would (and should) do so. But UI isn't a reason I'd use to justify a database schema refactor.

The priority on venue constraints is (as things currently stand) essential to how they work—if you have a constraint for a disabled person, and a constraint for a chief adjudicator, and you can't satisfy them both, the former should win. It also allows you to create "tiers" of constraints, where if you can't satisfy one for a given participant, you drop down to the next. I'm open to other ways of handling venue constraints, but I would need to be satisfied that the system can handle this requirement (for more than two "levels" of constraints).

tienne-B commented 5 years ago

It is nonetheless preferable to stick to one pattern when there are analogous structures. It is preferable for an ease of comprehension where we can point to one and understand other parts. In this, venue constraints and adjudicator conflicts are analogous in my view. I don't really have a view on which pattern is most appropriate (I do lean towards many tables as I'm the most used to that pattern and find the other "dirtier"). I wonder why they aren't in the same format, if they are analogous as posited.

Keeping the current schema, it should be possible to split into many pages, just specify the contenttype for each view. That would allow the drop-downs.

I do understand the necessity for prioritizing constraints :) Rather, it seems more as a property of the venue category itself if it's meant to be definitive or not. I am against having arbitrary numbers which may be inconsistent with the other costs that would have to be defined for #429. In that, constraints like "near GA" for CAs would not take precedence over accessibility constraints (which would point to another venue category). These constraints do not need to be ranked any further than whether we bar non-member venues or just discourage them (more than rotation) and it just adds complexity.

czlee commented 5 years ago

On prioritization, they actually do (I do this routinely). You might need to constrain one accessibility requirement (say, wheelchair) over another (say, on crutches), or one staff group (tab director) over another (CA), even though you're both constraining them to the same group. Again, this isn't hypothetical—I do this at every tournament. You'd be welcome to design another system that achieves this, but it has to permit this flexibility.

On consistency, as I say, inconsistent patterns in the code base currently are there because I'm trying out different patterns to get a feel for what they're like. In the long run, they should be resolved, like you say. But I don't want them to be "fixed" for the sake of being made consistent, until we know which pattern we want to follow. As a spare-time project we don't have the bandwidth to refactor constantly, so I'd rather not risk having to refactor twice, even if it means tolerating a weird schema in the meantime.

You can find a few blog posts by typing "should I use contenttypes" or "should I use generic foreign keys" into your favourite search engine. But, for what it's worth: I'm starting to be persuaded by this post by Luke Plant, which argues that we're probably overusing generic foreign keys currently (so we should split the tables, like you say). Have a read and let me know what you think?