learnweb / moodle-tool_lifecycle

:recycle: Extensible Moodle plugin for managing course life cycles, e.g., deprovisioning
GNU General Public License v3.0
20 stars 32 forks source link

Fix `sortindex` handling on step deletion #200

Closed olivabigyo closed 6 months ago

olivabigyo commented 6 months ago

There is a very subtle bug in the remove_from_sortindex function in classes/local/manager/step_manager.php. It looks like it should affect only the steps in the same workflow, but actually the workflowid is only passed as a parameter, but not actually used in the SELECT condition. So, what actually happens is that every workflow (that have enough) steps is changed when you delete a step from any workflow. Resulting in reordering of steps and other problems, like being unable to reorder steps anymore.

How to reproduce:

  1. Create two workflows. First with 3 steps, the Second with at least one step. (Choose whatever steps you want.)
  2. Remove the first step in the Second workflow.
  3. Check the sortindexes for the First workflow's steps in the DB, it's now 1, 1, 2 instead of 1, 2, 3.
  4. You can also notice problems by inspecting the First workflow on the UI. The order of the first two steps might have changed already (depending on how the DB orders the two steps with the same sortindex). But, if you try to move the first step down, it will become the third instead of the second. Etc.

This was very hard to debug, because you only notice that something happened to your workflows later, and don't associate this with changing things in another workflow.

Exactly the same bug is present in trigger_manager.php, though it's less problematic as the order of the triggers is not that important.

This PR contains the simple fix for the problem.

NinaHerrmann commented 6 months ago

Thank you so much for doing the whole packet (reporting, resolving etc. :heart_eyes: )