Multiple societies

[GKFX]

There's a fairly large chunk of code here, so in summary:

• This is a fresh start from the code I wrote over the summer. (A bit of stuff from then was copy-pasted but there are significant differences.)
• I'm effectively trying to store three things: links between Show entities and Society entities, unregistered society strings attached to a Show, and the display ordering of those Societies and strings for each show.
• The best way I could see of storing that is a conventional many-many join table between Show and Society, coupled with a column on the Show table called socs_list. socs_list stores a JSON array of strings (unregistered societies) and ints (Society ids); this gives the ordering and the unregistered societies.
• Some parts of the code care about all the societies; they call show->getPrettySocData() (directly or through a further wrapper) and get socs_list with id's replaced with actual Society objects and corrections made for any deviation from the join table.
• Other parts of the code (e.g. access control) only care about the registered societies; they call show->getSocieties() and ignore the JSON fun.
• Unfortunately ShowListener is an exception to this approach; it only has access to the JSON and the parsing code has to be effectively duplicated here.
• The API is continuing to return information in its old format in addition to new (using the getSoc*Legacy() functions); this should obviously be phased out in time but I haven't picked a date as yet.

[hoyes]

Maybe something to detect duplicate societies would be handy?

[CHTJonas]

Gave it a quick look over and had a play on the dev site - functionally all seems ok to me although my knowledge of Symfony isn't really good enough to give you a proper review of the code! 😝 My initial reaction to storing JSON in MySQL was 'ew' but that being said I can't think of a better way of doing this.

Part of me wonders whether this is actually stupidly overcomplicated and we're missing something really simple (no offence!). I wonder whether we should just create society entities in the database as & when shows are assigned to them, but only make them visible in the frontend (and searchable) when they have a visibility field set to true. And then just use the many-to-many mapping without the JSON. This seems conceptually much simpler and would also allow unregistered societies to become fully-registered ones really easily with one change to one row, rather than passing the JSON field in hundreds of rows in the DB to achieve the same thing.

Otherwise looks good to me. My vote would be to deprecate the legacy behaviour ASAP, possibly at the same time as #394 and #465, and maybe during Christmas?

[hoyes]

I think I maybe would have gone for an explicit intermediary table with the fields [id, society_name, society_id, order] (with nulls as appropriate for each entry), a bit like how acts_shows_people_link works to link shows and people. Doctrine apparently has a json_array data type which might simplify the current approach a little.

An explicit link table would mean the duplication thing above could be solved by just adding a UniqueEntity annotation...

I like @CHTJonas' plan for creating society rows for all entries, but it maybe increases the scope a little as far as #191 is concerned. There's a lot of good stuff in this PR already, so I'd be happy to merge this in more-or-less as is and maybe think about removing the JSON and "other_" fields as part of #465...

I think we'd have to forgo society ordering too in order to make do without a JSON field or explicit link table. Part of me would like to just accept that society names will always appear in alphabetical order and forgo manual sorting for simplicity (maybe that would help avoid real-life politics too?), but I can see how some people might find that contentious.

[GKFX]

An explicit intermediary table was the first approach I tried and it didn’t work particularly well. Perhaps it could’ve been made to work but my code was getting increasingly ugly – I think Charlie can confirm this!

As for the politics of society ordering, I would rather that people have all the tools they need to debate it amongst themselves rather than bringing our choice of alphabetical order into it.

The data currently sitting in other_society is a bit messy, so autogenerated Society entities would probably also be. Since we still need to store the ordering I would prefer to leave the current strings approach. There are JSON-handling functions in newer versions of MySQL, so in the long run it should be possible to run database queries that handle JSON without us having to parse it ourselves.

Will look into json_array.

[GKFX]

My approach to the legacy API would be the exact opposite to Charlie’s (sorry!), our API consumers are going to include students with limited time who quite possibly inherited all their code from a previous committee, so rapid change will be inconvenient. The legacy API is going to be returning the correct results anyway for basically all shows other than panto so it seems difficult to justify a rapid dropping of support.

However dropping any and all legacy APIs in one go seems very reasonable, so there’s just one breaking-changes release.

[CHTJonas]

Yeah fair point! To be honest I think we could count the number of applications out there that are using the particular data in question on one hand. (Incidentally this is one of the reasons I think we should enforce API registration). But I think I'm starting to go off-topic with this PR so can discuss elsewhere another time!

Yeah all seems good to me with what's been said above. 😄 (I know MariaDB/MySQL have JSON handling but that's something I'd typically associate almost with a NoSQL or document database)

[hoyes]

As far as this PR is concerned, something to detect duplicate societies would be nice but otherwise I'm happy.

Off-topic:

@GKFX do you still have your first branch lingering somewhere? I should've paid more attention at the time, but if I can think of any ideas for simplifying it that look worthwhile I might plonk it on a separate ticket... I guess it would require a lot of extra joins all over the place. I just had a go at defining a ManyToOne alongside a ManyToMany that use the same underlying table but Doctrine doesn't like that unfortunately...

At the moment, the "new developer" workflow relies on SQLite, so we'd have to revisit that if we ever start relying on MySQL/MariaDB-specific features. IMO given that shows are almost never going to contain more than 2 societies it's OK to do some processing in PHP here.

Totally agree about benefits of API registration - just a time/effort thing.

[GKFX]

Merged at the command line with a rebase and an extra commit.

@hoyes I've pushed it to here: https://github.com/GKFX/camdram/tree/multiple-socs. It can be best described as a learning experience.
I think adding safety features in MySQL is probably worthwhile as it prevents us from breaking the production DB, doesn't really matter if a quick SQLite checkout is a bit more breakable as long as it works okay. Currently checks are 100% PHP though; further checks is #540 since it requires a MariaDB upgrade.

[hoyes]

Just looked. For informative purposes only, I wonder if something like the below might have helped reduce some of the boilerplate/custom queries to fetch a show's societies. Plus if you can make the CollectionType work for you then it should handle all the collection diff'ing automatically during form submission.

class Show
{
...

/**
 * @ORM\OneToMany(targetEntity="ShowSocietyEntry")
 */
private $societyEntries
...
public function getSocieties()
{
    return $this->societyEntries->map(function($entry) { return $entry->getSociety(); });
}

The way you've gone though, $show->getSocieties(), which is probably the more common use case, is more direct so six of one etc. I reckon removing the "other_*" fields merits further discussion though, so I've created #541 so that doesn't get lost...

We're already relying on MySQL's cascade-on-deletion stuff (meaning that with SQLite you end up with orphaned rows) so I guess there's already a bit of a precedent there.