Page MenuHomeIn-Portal Phabricator
Differential D503

INP-1893 Denormalize "ChangeLogs" database table
AcceptedPublic
Actions

Authored by erik on Jan 22 2025, 12:20 PM.

Details

Reviewers
alex
Test Plan

Preparations

  1. Install previous version (522-B1)
  2. Login to adm.console as root
  3. Go to the Logs & Reports → Changes Log section
  4. Enable Changes Log by using special link above grid
  5. Logout
  6. Login to adm.console as root
  7. Go to the User Management → Administrators section
  8. Create administrative user, fill First Name, Last Name and Email fields
  9. Logout
  10. Login to adm.console as non-root
  11. Go to the User Management → Administrators section
  12. Edit administrative user, change some field value, save changes

Part 1 - upgrade test

  1. Make In-Portal Upgrade
  2. Login to adm.console
  3. Go to the Logs & Reports → Changes Log section
  4. Confirm, that Username, First Name, Last Name, Email columns contains proper data.
  5. Open debugger panel. Confirm, that no joins in sql request to ChangeLogs table.

Part 2 - clean install test

  1. Make In-Portal Clean install
  2. Login to adm.console as root
  3. Go to the Logs & Reports → Changes Log section
  4. Enable Changes Log by using special link above grid
  5. Logout
  6. Login to adm.console as root
  7. Go to the User Management → Administrators section
  8. Create administrative user, fill First Name, Last Name and Email fields
  9. Logout
  10. Login to adm.console as non-root
  11. Go to the User Management → Administrators section
  12. Edit administrative user, change some field value, save changes
  13. Go to the Logs & Reports → Changes Log section
  14. Confirm, that Username, First Name, Last Name, Email columns contains proper data.
  15. Open debugger panel. Confirm, that no joins in sql request to ChangeLogs table.

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.2.x
Lint
Lint ErrorsExcuse: Not fixing.
SeverityLocationCodeMessage
Errorcore/units/logs/change_logs/change_logs_config.php:116PHPCS.E.CodingStandard.Arrays.Array.SpaceAfterKeywordCodingStandard.Arrays.Array.SpaceAfterKeyword
Errorcore/units/logs/change_logs/change_logs_config.php:116PHPCS.E.Generic.PHP.LowerCaseKeyword.FoundGeneric.PHP.LowerCaseKeyword.Found
Unit
No Unit Test Coverage
Build Status
Buildable 11057
Build 3757: arc lint + arc unit

Event Timeline

erik created this revision.Jan 22 2025, 12:20 PM
erik requested review of this revision.Jan 22 2025, 12:20 PM
erik edited the test plan for this revision. (Show Details)Jan 22 2025, 12:21 PM
alex requested changes to this revision.Jan 22 2025, 12:39 PM

I haven't executed the test plan yet. Please make the proposed corrections/answer questions.

core/install/install_schema.sql
691–694

Please move the declaration of these columns after PortalUserId column.

core/install/upgrades.php
2558–2565 ↗(On Diff #1303)

Please explain pros/cons of this implementation compared to executing this SQL in the upgrades.sql file?

core/install/upgrades.sql
2990–2993

Please move the declaration of these columns after PortalUserId column.

2994

Is it possible for Guest user to cause change log record creation?

If so, then please also handle this across the Differential Revision.

core/kernel/db/db_event_handler.php
2107–2109 ↗(On Diff #1303)

Please remove.

No need to assign empty values, because they match default values in these columns.

2111 ↗(On Diff #1303)

Please replace with:

$cache[$user_id] += $user_data;

Array concatenation would work faster in this case.

2122–2131 ↗(On Diff #1303)

Please replace with:

if ( $user_data !== false ) {
    $cache[$user_id] += $user_data;
}
  • Array concatenation would work faster in this case.
  • No need to assign empty values, because they match default values in these columns.
This revision now requires changes to proceed.Jan 22 2025, 12:39 PM
erik marked an inline comment as done.Jan 23 2025, 4:54 AM
erik added inline comments.
core/install/upgrades.php
2558–2565 ↗(On Diff #1303)

Not working in upgrades.sql, because no table prefix replacement for joining table Users.

erik updated this revision to Diff 1304.Jan 23 2025, 5:19 AM
erik marked 6 inline comments as done.
erik edited the test plan for this revision. (Show Details)

Done all requested improvements

erik added inline comments.Thu, Jan 23, 9:27 AM
core/install/upgrades.sql
2994

Not possible change log for guest user, because session variable '_SessionLogId_' is required for change log logging. And this variable is set only after login.

alex requested changes to this revision.Thu, Jan 23, 10:32 AM

The test passed. However, please make these minor adjustments.

core/install/upgrades.php
2558–2565 ↗(On Diff #1303)

For such cases use <%TABLE_PREFIX%> placeholder as done in the below example (from the upgrades.sql file):

INSERT INTO Permissions (Permission, GroupId, PermissionValue, Type, CatId)
SELECT 'in-portal:configemail.add' AS Permission, GroupId, PermissionValue, Type, CatId
FROM <%TABLE_PREFIX%>Permissions
WHERE Permission = 'in-portal:configemail.edit';
core/kernel/db/db_event_handler.php
2085 ↗(On Diff #1304)

Please correct a typo in the method parameter description.

2114 ↗(On Diff #1304)

Please only use TABs for indentation.

This revision now requires changes to proceed.Thu, Jan 23, 10:32 AM
erik updated this revision to Diff 1305.Thu, Jan 23, 10:45 AM

Done all requested minor fixes

alex accepted this revision.Fri, Jan 24, 3:10 AM
This revision is now accepted and ready to land.Fri, Jan 24, 3:10 AM
alex updated this revision to Diff 1306.Fri, Jan 24, 3:11 AM

Minor SQL formatting changes.

alex retitled this revision from INP-1893 Don't use JOIN's in "Change Logs" grid to INP-1893 Denormalize "ChangeLogs" database table.Sun, Jan 26, 1:57 PM
alex updated this revision to Diff 1311.Wed, Jan 29, 4:42 AM

Rebase on top of D504.

alex updated this revision to Diff 1312.Wed, Jan 29, 6:21 AM
  1. extract "\ChangeLogHelper::getUserFields" from the "\ChangeLogHelper::getAddFields" for easier customization
  2. change cache key of the \ChangeLogHelper::getAddFields method to include also session_id
alex updated this revision to Diff 1313.Wed, Jan 29, 6:29 AM

Ensure new identical field order in both unit config and database table.

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 1130316823Jan 22 2025, 12:20 PM
★
★
Diff 2130416823Done all requested improvementsJan 23 2025, 5:19 AM
★
★
Diff 3130516823Done all requested minor fixesThu, Jan 23, 10:45 AM
★
★
Diff 4130616823Minor SQL formatting changes.Fri, Jan 24, 3:11 AM
★
★
Diff 5131116824Rebase on top of D504.Wed, Jan 29, 4:42 AM
★
★
Diff 61312168241. extract "\ChangeLogHelper::getUserFields" from the "\ChangeLogHelper…Wed, Jan 29, 6:21 AM
★
★
Diff 7131316824Ensure new identical field order in both unit config and database table.Wed, Jan 29, 6:29 AM
★
★

Diff 1311

View Options

core/install/install_schema.sql

Loading...
View Options

core/install/upgrades.sql

Loading...
View Options

core/units/helpers/ChangeLogHelper.php

Loading...
View Options

core/units/logs/change_logs/change_logs_config.php

Loading...
In-Portal CMS · In-Portal Community Pages · In-Portal Issue Tracker · In-Portal Repository Browser · Quality Assurance · In-Portal CI Server