Page MenuHomeIn-Portal Phabricator

INP-1893 Don't use JOIN's in "Change Logs" grid
Needs RevisionPublic

Authored by erik on Wed, Jan 22, 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 & ReportsChanges 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 ManagementAdministrators 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 ManagementAdministrators 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 & ReportsChanges 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 & ReportsChanges 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 ManagementAdministrators 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 ManagementAdministrators section
  12. Edit administrative user, change some field value, save changes
  13. Go to the Logs & ReportsChanges 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: all code style fixes is not part of this task
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 11049
Build 3749: arc lint + arc unit

Event Timeline

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

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

core/install/install_schema.sql
687–690

Please move the declaration of these columns after PortalUserId column.

core/install/upgrades.php
2558–2565

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

Please remove.


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

2111

Please replace with:

$cache[$user_id] += $user_data;

Array concatenation would work faster in this case.

2122–2131

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.Wed, Jan 22, 12:39 PM