Skip to content

Clean up $sub logic in Control

Fridtjof requested to merge clean-up-sub-logic into master

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:

  1. IndexController finds the controller to call (based on the page= argument) and instantiates it.
  2. 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")).
  3. The first part (before the '/') will be assigned to the $sub field.
  4. The last part (after the '/' if one is there, or the first part) is assigned to the $sub_func field.
  5. 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.
  6. 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.

  1. Checkout branch locally
  2. Login as foodsaver or foodsharer
  3. Use any link in the UI to get to the upgrade page
  4. 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 notes refactoring
  • 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.
Edited by Fridtjof

Merge request reports

Loading