Add capability to add multiple request forms

[stwalkerster]

Request forms are per-domain, and can be routed to specific queues.

Currently, the form fields cannot be changed, but all the text can be. Formatting is done via Markdown in a safe rendering mode to avoid any code injection, but to permit the usage of Bootstrap styling for things like alert boxes.

This also shows on the request zoom page where this request has come from, plus leaving a comment indicating why the request is in that queue:

Interesting things do happen if both a custom form and a defer-type ban are set:

Fixes #517 #604 #605

[stwalkerster]

Deployed to https://accounts-dev.wmflabs.org/demo/internal.php if you want a play around with it

[dqwiki]

I'm worried about the potential conflict this will create with domain management. If we intend to deploy domain management how would these requests be sorted by domain?

[stwalkerster]

I'm worried about the potential conflict this will create with domain management. If we intend to deploy domain management how would these requests be sorted by domain?

So this is actually built primarily with domains in mind, allowing the creation form to be customised per-domain. Almost everything is (will be) split per-domain, including requests and request queues (though that split isn't enforced yet). A request queue is created in a specific domain, and will deposit requests in a domain-specific request queue. You will only see request queues from your current domain.

[FastLizard4]

My review so far, which consists of questions mostly just to satisfy my curiosity/help my understanding/memory of how all this works

[FastLizard4]

Out of curiosity, why these changes?

[stwalkerster]

Because PHP.

Actually, because the get*() methods in RequestForm are type-restricted to be a string, and PHP defaults variables to null. Since these are required fields, I don't really want to make them nullable (the string? type annotation)

For both creation and edits, PageRequestFormManagement calls the populateFromObject method to make it easier to set all the field assignments on the template, but for new objects it's done via populateFromObject(new RequestForm()), thus all the fields are default values, and (without this change) the fields are all null which doesn't match the type annotation on the get*() methods. For a recent example of this sort of thing happening in prod, see #754

[FastLizard4]

What's this FIXME for?

[stwalkerster]

In many places, I've hard-coded the domain ID 1 until everything supports retrieving via domain and we start actually enforcing domains on login sessions. These FIXMEs are the locations where the domain is hard-coded so it's much easier to find them later.

They will be fixed probably in #600, which is part of the multi-domain push (#532)

[FastLizard4]

I see both edit() and view() have a block like this; do preview() and create() need one too? Or do I just not remember how the access control checks work? 😅

[stwalkerster]

This check is to make sure the user's current domain is able to see this specific object. It's basically one of the checks I wanted to add in #600 (see your other comment about the FIXMEs).

Since this is a new object, create() doesn't need this check as the object will be created in the user's current domain (see L81).

Since preview never actually calls the database and just populates from what's in the user's session, there's no object to check against. The user must have been able to see an object (or create an object) already for this to work.

[FastLizard4]

Is this redundant with the preview() method?

[stwalkerster]

If I remember this code correctly, preview() launches an iframe under the edit() mode. This fragment of code handles the refresh (submit!) of the edit() mode, and sets up the session variables to pass to the iframe. The preview() mode in the iframe then actually handles the render.

Not redundant, two parts of the same functionality.

[FastLizard4]

Out of curiosity, why updateversion of 9?

[stwalkerster]

I actually have no idea what I was thinking four months ago!

I think what happened was I imported the existing template, made tweaks to it through the GUI to convert from HTML to markdown, and then exported the row as a SQL insert into the migration script, and it just happened to be version 9.

This makes no appreciable difference though, updateversion is an unsigned 32-bit integer so as long as we don't make over 4 billion(ish) changes to this request form, a difference of 9 isn't going to matter 😀

[FastLizard4]

Code looks good to me (though since it's been so long since reviewing ACC code I can't guarantee I haven't missed something subtle, like the exceptions missing constructor args). I did notice an issue during my testing, but given the nature of the multi-domain work I imagine it might very well be expected at this point. Let me know if it is and I'll approve!

If you create a fresh domain and request form within that domain, but don't specify a queue for that request form to go to (leaving it at "(Use default queue)"), you get an exception when trying to submit a request. The exception occurs at line 112 of PageRequestAccount.php:

$request->setQueue(RequestQueue::getDefaultQueue($database, $domain)->getId());

...Because getDefaultQueue() returns false, and you can't call getId() on a boolean. Note that I did create a queue for my new domain, but I didn't set it as the default for requests. Trying to set an explicit default queue for the form results in a "The chosen queue does not exist or is disabled" error message, seemingly because the created queue doesn't belong to the domain I created it in.

[stwalkerster]

Yep, that's expected at the moment - many things don't work with domains yet - most of the work has been getting the tool to a point where the "make domains work" PR can be as small as possible. When you create several domains and try to use the second domain, things do break due to the hardcoding of domain id 1 almost everywhere

[FastLizard4]

Makes sense. !