SDSM:Historical Changes

This document consists of multiple parts; for a directory to all of the parts, see SDSM:Index.

Description

This part of the SDS Modernization (SDSM) document enumerates a not necessarily exhaustive list of historical changes or improvements that were made to SDS, made by Darren Duncan if by whom is not otherwise specified.

It exists to provide visibility into historical progress of the SDS Modernization (SDSM) project about which this document otherwise mainly just describes the current state of SDS.

This list often but not always corresponds to the list of Git pull requests at https://git.smus.ca which most readers of this document are not privileged to see directly.

RETURN

SDS Laravel: Fixes to Broken Behavior

2024 Apr 18: User Preferences Screen Fails to Display

Context: User darren.duncan attempted to visit their User Preferences screen, example urls https://api.smus.ca/User/Preferences (production) and https://api-latest.smus.ca/User/Preferences (preview).

Observed Behavior: In production, a non-graceful generic 500 internal server error message was displayed rather than the expected User Preferences screen. In preview, the revealed underlying symptom was that the code line in PreferencesController of $addresses = $user->guardian->addresses; was dying with Attempt to read property addresses on null. The problem occurred for user darren.duncan but not for other users.

Expected Behavior: The User Preferences screen should always display, regardless of whether the user in question is a guardian or has guardians. If some functionality of the screen is not applicable for users that aren't or don't have guardians, the screen should either hide that functionality or explain why it doesn't apply or both.

Applied Solution: The dying code line was replaced with code that only tried to fetch and return an address list when it was valid to do so, and it returned an empty list otherwise. As a result, the User Preferences screen never failed to display under this scenario.

Caveat: While the User Preferences screen no longer fails to display, as of the completion of this fix, it still remained true that the functionality to actually display the fetched address list in question was never implemented, and so for every user, the screen carried the placeholder message Shows address info instead of each individual address.

RETURN

2024 May 14: Main Menu Bar Heavy Database Use Very Slow Page Loads

SDS Laravel currently relies on the "pages" database table to canonically define its app main navigation menus and it currently has 201 records where each defines a menu item.

The main menus are hierarchical, and the "order" field of each "pages" record serves double duty to define the parent/child aka super/sub relationships of menu items as well as their display order. The "order" of a record is used as a unique key for the record for this purpose, as well as a foreign key from child to parent.

Each "order" is a positive integer N such that:

• The result of log-100(N) is the item's tree level.
• The result of div-by-100(N) is the item's parent item's "order" in the tree.
• N itself is the item's display order relative to its siblings.

For example, a root level menu item has an "order" in 1..99, and for the root item with an "order" of 3, all of its children would have "order" in 301..399, and the child of 303 would have "order" in 30301..30399 etc.

(Very notably, the actual primary key field of "pages" is NOT used for describing menu parent/child relationships in conjunction with a second field, as is the more typical practice in other databases.)

SDS Laravel reads the entire "pages" database table and constructs a hierarchical menu in 2 different circumstances. The primary and most important circumstance is for nearly every single screen, those where the user is logged in, and it is a normal screen, and not an API, a menu for navigation appears at the top of each screen. The secondary circumstance is on the "System Settings"->"Pages" screen that is used to edit this database table, where the hierarchy is rendered as a left column.

Prior to the changes of this task, the hierarchical menu assembly was performed in a very inefficient manner such that the application made a separate database query for every single tree node to fetch its children, meaning that it was making around 130 queries for nearly every screen displayed to the user just for making the navigation menu alone. And on the "Pages" screen it was making around 430 additional queries (total around 560) just to display the menu hierarchy on that one screen. For context, the sum total of all other database queries for all other purposes on a single screen was around 20-30.

Compounding the above query-per-node inefficiency was another kind of inefficiency where, for each menu node, the same query for its children was often done up to 3 times (or up to 5 times), where only the last query actually enumerated the list of child nodes, and prior times simply tested if there were any children, and if there were children, the query was run again to actually use them.

Following the changes of this task, the hierarchical menu assembly only involved 2 SQL queries as overhead for nearly every screen, and 2 more on the "Pages" screen, because it now selects all the "pages" records at once rather than a handful or zero at a time, and uses the same fetched records when building every menu node. The records are grouped when fetched in a PHP array of arrays, the root array keyed by parent "order" values, and the array value for each being an array of its child "pages" records.

This change had a massive impact on the user-visible screen loading speed of SDS Laravel, particularly when a developer runs the application and its database on opposite ends of a VPN tunnel where the added latency of each database call magnified the overhead time of each database query.

For example, before the changes, running the app on a local machine at home, where it talked to a database on a school server, took about 23..25 seconds (about 150 queries) on the fastest screen with a menu to display on that screen, and after the changes it took about 3.5-3.9 seconds (about 20 queries). And on the "Pages" screen it took about 1.5 minutes (about 575 queries) to display the screen before the changes, and about 4.5 seconds (about 20 queries) after the changes.

For a typical non-developer user scenario which didn't have that kind of added VPN latency, the time to load a faster screen before the changes was around 1-2 seconds. After the changes it was about 0.1-0.2 seconds, including on the "Pages" screen.

This task kept its changes about as minimal as possible while making this change. There was no significant change to how screens or menus were defined, and anything else related to the application design, aside from the minimal needed to consolidate the database queries. While there is a lot of room to make other improvements to how menus are implemented, other such changes are out of scope for this task.

Here are specific numbers for before/after, school/home-VPN, on 4 screens.

SDS Laravel/API Screen: /

• Blade Views Rendered: 1
• Database Queries Run Before: 0
• Database Queries Run After: 0
• Eloquent Models Used Before: 0
• Eloquent Models Used After: 0
• HTTP Page Load Time at School Before: about same
• HTTP Page Load Time at School After: 58.8ms,59.9ms,55.3ms,59.3ms,63.1ms
• HTTP Page Load Time at Home/VPN Before: about same
• HTTP Page Load Time at Home/VPN After: about same

SDS Laravel/API Screen: /home

• Blade Views Rendered: 3
• Database Queries Run Before: 192
• Database Queries Run After: 56
• Eloquent Models Used Before: 455
• Eloquent Models Used After: 475
• HTTP Page Load Time at School Before: 672ms,671ms,734ms,725ms,617ms
• HTTP Page Load Time at School After: 362ms,416ms,364ms,271ms,256ms
• HTTP Page Load Time at Home/VPN Before: about 26.39s,24.9sX2,25.0s
• HTTP Page Load Time at Home/VPN After: about 5.63s,5.65s

SDS Laravel/API Screen: /Event/Calendar/33297

• Blade Views Rendered: 4
• Database Queries Run Before: 155
• Database Queries Run After: 19
• Eloquent Models Used Before: 200
• Eloquent Models Used After: 220
• HTTP Page Load Time at School Before: 462ms,487ms,589ms,493ms,491ms
• HTTP Page Load Time at School After: 126ms,131ms,125ms,121ms,125ms
• HTTP Page Load Time at Home/VPN Before: about 23.61s,24.34s,24.60s
• HTTP Page Load Time at Home/VPN After: about 3.55s,3.62s,3.90s

SDS Laravel/API Screen: /System/Page

• Blade Views Rendered: 34
• Database Queries Run Before: 581
• Database Queries Run After: 20
• Eloquent Models Used Before: 779
• Eloquent Models Used After: 436
• HTTP Page Load Time at School Before: 1.86s,1.85s,1.79s,1.88s,1.80s
• HTTP Page Load Time at School After: 170ms,207ms,173ms,316ms,208ms
• HTTP Page Load Time at Home/VPN Before: about 1.7min,1.5min
• HTTP Page Load Time at Home/VPN After: about 4.45s

RETURN

SDS Laravel: Internal Design Changes

2024 Apr 19: Consolidate Eloquent model classes to /app/Models

An idiomatic PHP Laravel project has its main files grouped into the specific hierarchy of folders documented here:

https://laravel.com/docs/11.x/structure

Of particular relevance, the folder /app/Models is where all of the Laravel Eloquent model classes go.

Prior to the performance of this task, SDS Laravel conformed to the idiomatic folder layout in all ways except with respect to the Eloquent model classes, which it had instead spread across about a dozen /app/FooModel folders.

This task moved/renamed all of the Eloquent model classes, such that for each class /app/FooModel/Bar.php, it was moved/renamed to /app/Models/Foo/Bar.php. While that described nearly all of those classes, a handful started with other names, but they were appropriately gathered under /app/Models too.

This task also included a normalization of how first-party classes were referred to in other code, such that any which weren't referred to by their fully-qualified names before were referred to in that way after.

RETURN

2024 May 8: Sever Coupling of PHP Class Names to Database Data

Laravel Eloquent has a particular more-advanced feature for defining relationships between model classes that it refers to as a polymorphic relationship, such that a relationship is not just between 2 specific model classes, but rather between 1 model class and 1 set of all model classes that fulfill a particular role.

https://laravel.com/docs/8.x/eloquent-relationships#polymorphic-relationships

Prior to the performance of this task, SDS Laravel utilized such an Eloquent relationship, between Group on one side, and the set comprising Page and SubPage on the other side.

And it did so in a design-flawed way that led to tight coupling between the exact fully-qualified PHP class names of the latter 2 model classes, and the database data, such that those fully-qualified class names (App\SubModel\Page and App\SubModel\SubPage) were stored in the database (specifically in the group_pages table's class_type column) and the fact that these matched was relied on by the application for correct behavior.

This tight coupling manifested as broken app behavior when attempting to move/rename the Page and SubPage classes (to App\Models\Sub\Page and App\Models\Sub\SubPage), because then their names no longer matched what was in the database.

The most common kind of breakage was that, for almost every app screen that was gated with a user group privilege check for the screen, the app thought no user had any privilege to use it; for example, this error message displayed when trying to app general search screen:

   No access to Search.index
   You do not appear to have permission to access https://api-latest.smus.ca/Search as [User's Name]. If you believe you should have access
   to open a help ticket request permission, or you can go back / home and try again.

And had users been able to get past that, certain other screens or functionality that rely on displaying or editing user group privileges for pages and subpages would have had problems, such as failing to see existing privileges made before the rename (they might have worked to re-add the privileges, then using the new hard-coded class names in the database).

Without correcting the design flaw, renaming these model classes would have required a corresponding database migration to replace the old hard-coded-in-data PHP class names with their new ones, which carried more complexity and risk.

The more specific design flaw was the use of Eloquent's morphToMany() relationship builder without specifying an explicit constant string to represent each morphed class in the database, resulting in Eloquent defaulting to using its fully-qualified PHP class name instead.

Both Page and SubPage had a copy of this method (this version is post-rename) which was affected:

   public function groups(){
       return $this->morphToMany(\App\Models\Sub\Group::class, 'class', 'group_pages');
   }

This task made the simplest possible fix to this design flaw by explicitly configuring Eloquent to use an explicit constant string per each morphed class, by adding the following in the file app/Providers/AppServiceProvider.php:

   Relation::enforceMorphMap([
       'App\SubModel\Page' => 'App\Models\Sub\Page',
       'App\SubModel\SubPage' => 'App\Models\Sub\SubPage',
       // TODO: Migrate database to use non-PHP-class-looking slugs
       // like following instead so they don't look tightly coupled.
       // 'page' => 'App\Models\Sub\Page',
       // 'sub_page' => 'App\Models\Sub\SubPage',
   ]);

This was the simplest possible fix, and ostensibly the most elegant, by working with the system, using an Eloquent feature expressly designed for this very scenario, rather than say trying to avoid using the morph features (although doing so may be better longer term), and also by not requiring any database changes, so it works with the database as-is.

https://laravel.com/docs/8.x/eloquent-relationships#custom-polymorphic-types

Following the changes of this task, there was still room for an even more elegant solution to follow that alters the database to use a non-php-class-looking constant in the database data to represent each class, so the app isn't forever using the old class name there, but that was considered higher complexity and risk and suited to be done as a separate task another day.

Note that a consequence of using Relation::enforceMorphMap() is that it makes Eloquent treat the entire app more strictly, such that if we try to use morphToMany() or similar Eloquent methods with any other classes besides Page and SubPage, it will require us to add an entry to the above list for it, rather than silently defaulting to the old behavior for other unspecified classes. I consider this strictness to be a good thing.

RETURN

SDS Laravel: Changes to Third-Party Dependencies

2024 May 3: Stop requiring dbal, revisionable-upgrade, laravel-cors

Prior to the performance of this task, the SDS Laravel Composer config file composer.json explicitly required these 3 third-party PHP library dependencies, which were not actually used by the app:

• doctrine/dbal (^3.5)
• fico7489/laravel-revisionable-upgrade (*)
• fruitcake/laravel-cors (^2.0)

This task updated composer.json to remove those unused explicit dependencies, which also had the effect of removing 6 other Composer-resolved dependencies.

RETURN

2024 May 6: Reflect rename of fzaninotto to fakerphp

This task updated composer.json to reflect that the third-party PHP library dependency fzaninotto/faker was renamed to fakerphp/faker (in fact, that rename occurred prior to the release of Laravel 8), and also require its latest version.

RETURN

2024 May 6: Upgrade PHPUnit from 9.x to 10.x

This task updated composer.json to require the next/newest major version of the PHP dev dependency PHPUnit from 9.6.19 to 10.5.20.

RETURN