Clean up $sub logic in Control
What does this MR do?
Currently, the entire logic around the two variables $sub and $sub_func is very messy and hard to understand. The flow goes something like this:
- IndexController finds the controller to call (based on the page= argument) and instantiates it.
- In the Control base constructor, the sub= argument is taken, split at a '/' that is not there in 99% of the cases (I checked using a project wide search using this regex:
sub=[a-zA-Z0-9]+/
and only URLs related to settings and the role upgrade pages came up (all in all, "upgrade/up_fs", "upgrade/up_bip" and "upgrade/up_bot")). - The first part (before the '/') will be assigned to the $sub field.
- The last part (after the '/' if one is there, or the first part) is assigned to the $sub_func field.
- Back to the IndexController: it checks for an a= argument, and calls any method with the same name on the controller. If a method with that name does not exist, or a= isn't present, the "index" method will be called instead.
- After that, $sub_func is checked - if there is a method with its name, it is executed too.
Now, for 99% of all cases (where sub= does not contain a '/'), $sub_func has the same value as $sub! SettingsControl was the only place I could find where any sub= values with a '/' are encountered. To make this even work, an empty method called "upgrade" is defined, with the actual work being done in "up_bip", "up_fs", "up_bot".
This MR gets rid of all that noise by changing all URLs to just "up_fs" instead of "upgrade/up_fs" (which actually works without any further changes, because there was no code in the "upgrade" method anyway) and only leaves $sub.
NOTE: There was one link in a database migration for a content text (ID 33). That will have to be changed by someone with orga rights. Because this MR is not actually required for the link to work without "upgrade/", that can be done right now, and does not have to wait for the finished release!
How confident are you it won't break things if deployed?
very confident, tested locally - feel free to solve a quiz with user1@example.com
to verify
How to test
Steps a reviewer can take to verify that this MR does what it says it does e.g.
- Checkout branch locally
- Login as foodsaver or foodsharer
- Use any link in the UI to get to the upgrade page
- Verify that the link takes you there as expected.
Checklist
-
Tests are already there (QuizCest, CreateCompanyAsFoodsaverCest, and maybe more) -
no unrelated changes -
fixed any affected links in the production database - someone with Orga permissions has to do this -
asked someone for a code review -
set a "for:" label to indicate who will be affected by this change -
use "state:" labels to track this MR's state until it was beta tested -
added an entry to CHANGELOG.md -
add a short text that can be used in the release notesrefactoring -
Once your MR has been merged, you are responsible to create a testing issue in Beta Testing Repo: - Consider writing a detailed description in German.
- Describe in a few sentences, what should be tested from a user perspective.
- Also mention different settings (e.g. different browsers, roles, ...). how this change can be tested.
- Be aware, that also non technical people should understand.