feat: copy questions from another form - #3636
Conversation
Rebuilding the same question set by hand for each new form is tedious, and the server already knew how to clone a question together with its options - it simply refused to do so across forms. That restriction is lifted for forms the user is allowed to EDIT. Simply dropping the same-form test would not have been safe: any question could then be read out of any form by guessing ids, so the source form is permission-checked. One bug had to be fixed for this to work at all: Question::read() carries the source question's formId, so a clone taken from another form would have been created back in that form rather than in the target one. The target id is now set explicitly. A dialog lists the forms the user can edit, then the questions of the chosen one, with select-all. Copies are made sequentially because each is appended at the end of the form, and issuing them in parallel would give an unpredictable resulting order. A partial failure reports that some questions were copied rather than implying none were. No schema change, and no change to the API surface: the existing fromId parameter and its documented behaviour are unchanged for same-form cloning. Signed-off-by: global-prog <raqeeb@uosamarra.edu.iq>
0040fcd to
2ea0955
Compare
|
@global-prog Thanks for your contribution. 👍🏻 However, please create an issue/feature request before starting implementation the next time so that we can discuss it. Also please add the AI declaration to your description if you used AI for this. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Copying appends at the end by reading the order of the target form's last question, and end() of an empty list is false rather than a question, so the call on it was fatal. That could not happen while copying was limited to the same form, which always held at least the source question. Now that the source may be another form, copying into a new form with nothing in it yet -- the ordinary case -- failed with a server error. It now starts at 1, as adding a new question to an empty form already does. Adds tests for copying from another form, into an empty form, and from a form the user cannot edit, which also covers the permission check the first commit added. Signed-off-by: global-prog <raqeeb@uosamarra.edu.iq>
|
Thanks! Understood, I'll open a feature request first for anything further. While adding tests I found that copying into a form with no questions yet failed with a server error: the copy is appended after the target form's last question, and there isn't one. That couldn't happen while copying was limited to the same form, but it's the ordinary case for this dialog, so it's fixed in the latest commit, along with tests for copying from another form, into an empty form, and from a form without edit rights. The AI declaration is added to the description. |
Summary
Rebuilding the same set of questions by hand for every new form is tedious. The server already knows how to clone a question together with its options — it simply refuses to do so across forms.
This lifts that restriction for forms the user is allowed to edit, and adds a small dialog to pick them.
Why the restriction could not just be removed
Two things needed handling:
Permission. Dropping the same-form test alone would let any question be read out of any form by guessing ids, so the source form is now checked with
getFormIfAllowed($sourceFormId, PERMISSION_EDIT).A latent bug.
Question::read()includes the source question'sformId, and the clone path passes that straight intoQuestion::fromParams(). For same-form cloning that is harmless because the value matches; for a cross-form copy the new question would have been created back in the source form. The target id is now set explicitly.UI
A dialog lists the forms you can edit, then the questions of the chosen one, with select-all.
Copies are issued sequentially because each is appended at the end of the form — in parallel the resulting order would be unpredictable. If some copies succeed and one fails, the message says some were copied rather than implying none were.
Scope
fromIdparameter and its documented behaviour are unchanged for same-form cloning, soopenapi.jsonis unaffectedlib/Controller/ApiController.phpchanges by 10 insertions / 2 deletionsTesting
npm run lintandnpm run stylelintclean,php -lcleanmainwith no errorsHappy to drop the dialog and ship only the backend change if you would rather design the entry point differently.
🤖 AI (if applicable)