Unexpected output: Affected paths: relative - resubmit patch.
⚙ D165 INP-1510 Allow rotation of "Session Log" and "Changes Log"
Page MenuHomeIn-Portal Phabricator
Differential D165

INP-1510 Allow rotation of "Session Log" and "Changes Log"
ClosedPublic
Actions

Authored by erik on Nov 6 2015, 11:24 AM.

Details

Reviewers
alex
Commits
rINP16329: Fixes INP-1510 - Allow rotation of "Session Log" and "Changes Log"
Summary

Ability to rotate "Session Log" and "Changes Log"

Test Plan

Part 1

  1. go to Configuration → Website → Advanced section
  2. ensure that "Keep "Session Log" for" is set to "Forever"
  3. ensure that "Track database changes to change log" is checked, save
  4. log out
  5. log in
  6. log out
  7. log in
  8. go to Tools → Query Database section
  9. launch SQL like: UPDATE inp_UserSessionLogs SET SessionEnd = -5 WHERE NOT ISNULL(SessionEnd);
  10. go to Logs & Reports → Session Log section
  11. confirm, that Session end date for all ended sessions is about 01/01/1970
  12. run in-portal scheduled-task:run rotate_session_logs command from shell while in the project directory
  13. go to Logs & Reports → Session Log section
  14. confirm, that oldest sessions remains in the list

Part 2

NOTE: Please repeat below steps for each of "Keep "Session Log" for" values, excepting "Forever". That is - for 1 day (86400 seconds), 1 week (604800 seconds), 2 weeks (1209600 seconds), 1 month (2629800 seconds as average), 3 months (7889400 seconds as average), 1 year (31557600 seconds as average), 2 years (63115200 seconds as average), 3 years (94672800 seconds as average), 5 years (157788000 seconds as average) values of "Keep "Session Log" setting.
  1. go to Configuration → Website → Advanced section
  2. ensure that "Keep "Session Log" for" is set to "1 week"
  3. change "Enable SEO-friendly URLs mode (MOD-REWRITE)" setting, save
  4. go to Logs & Reports → Changes Log section
  5. confirm, that last change of Configuration is in the list
  6. log out
  7. log in
  8. go to Tools → Query Database section
  9. launch SQL like: UPDATE inp_UserSessionLogs SET SessionEnd = UNIX_TIMESTAMP() - #seconds# WHERE NOT ISNULL(SessionEnd), where #seconds# must be replaced with corresponding integer value from note above
  10. go to Logs & Reports → Session Log section
  11. confirm, that Session end date for all ended sessions is about ("Current Date" - "period, selected from note above for test")
  12. run in-portal scheduled-task:run rotate_session_logs command from shell while in the project directory
  13. go to Logs & Reports → Changes Log section
  14. confirm, that list is empty
  15. go to Logs & Reports → Session Log section
  16. confirm, that only current session is in the list

Part 3

  1. make 2 or more outdated sessions with change log records (login and logout several times, make some changes and save)
  2. launch SQL like: UPDATE inp_UserSessionLogs SET SessionEnd = -5 WHERE NOT ISNULL(SessionEnd)
  3. go to Logs & Reports → Session Log section
  4. confirm, that Session end date for all ended sessions is about 01/01/1970
  5. go to Configuration → Website → Advanced section
  6. ensure that "Keep "Session Log" for" is set to "1 week"
  7. change system code temporarily - set $limit = 1 in place of $limit = 100 in SessionLogEventHandler::OnRotate event
  8. run in-portal scheduled-task:run rotate_session_logs command from shell while in the project directory
  9. go to Logs & Reports → Session Log section
  10. confirm, that only current session is in the list
  11. go to Logs & Reports → Changes Log section
  12. confirm, that only last session changes are in the list
  13. restore system code - set $limit = 100 in SessionLogEventHandler::OnRotate event

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.3.x
Lint
Lint ErrorsExcuse: full event handler re-formatting is not a part of this task
SeverityLocationCodeMessage
Errorcore/units/logs/session_logs/session_log_eh.php:190PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Unit
No Unit Test Coverage
Build Status
Buildable 249
Build 249: arc lint + arc unit

Event Timeline

erik updated this revision to Diff 404.Nov 6 2015, 11:24 AM
erik retitled this revision from to INP-1510 Allow rotation of "Session Log" and "Changes Log".
erik updated this object.
erik edited the test plan for this revision. (Show Details)
erik added a reviewer: alex.
erik added 1 JIRA issue(s): INP-1510.
alex requested changes to this revision.Nov 9 2015, 5:19 AM
alex edited edge metadata.

Aside from inline comments following needs to be fixed in the test plan as well:

  1. go to Configuration → Website → Advanced section
  2. ensure that "Track database changes to change log" is checked
  3. ensure that "Keep "Session Log" for" is set to "1 week"
  4. change "Enable SEO-friendly URLs mode (MOD-REWRITE)" setting, save
  5. log out
  6. launch some SQL to offset all non-null UserSessionLogs.SessionEnd values more than "1 week" to the past

Above mentioned part of "Part 1" from test plan will have no effect, because the Track database changes to change log system setting change will only have effect on newly created session (admin session, where it was changed isn't affected).

  1. launch some SQL to offset all non-null UserSessionLogs.SessionEnd values more than "1 week" to the past

The test is performed by human, that isn't aware of how changed functionality works now or before and ideally by a non-developer. Therefore the test plan items must be as concrete is possible (not some SQL as in this case). Please (in test plan):

  • mention exact SQL
  • explain how to adapt SQL to always work (e.g. replace timestamp to today, etc.)
  • explain how to verify, that SQL really worked
  1. edit "rotate_session_logs" record, change "Next Run On" to some date in the past
  2. wait until "rotate_session_logs" scheduled task performs it's job

In case, when user haven't configured CRON and set scheduled task to be executed from cron this will never work. Instead I recommend running in-portal scheduled-task:run rotate_session_logs command from shell while in the project directory.

  1. confirm, that only last change of Scheduled task is in the list

Update this to still work even if scheduled task invocation code was changed.

core/units/logs/session_logs/session_log_eh.php
148

With such a small limit there will be a lot of database queries (one per database record) executed.

149

Please use boolean data type here (e.g. true and false) not 1 and 0.

The variable value doesn't represent quantity, but integer data type value is being used.

150

Please rename to $session_log_config.

Makes code easier to read.

151

Please move variable closer (but above cycle) to the usage point.

Variable is declared too far ahead of it's usage place.

152–155

Please rename to $session_log_select_sql.

Makes code easier to read.

157–169

Please:

  1. create helper, that would accept prefix_special & event and return matching temp handler instance
  2. use method here to get 2 temp handler instances

Makes code easier to read.

158–162

Please rename to $session_log_temp_handler.

Makes code easier to read.

171

Please:

  1. transform into do ... while cycle
  2. remove the $may_have_session_log

The do ... while variation of while cycle is best to be used, when cycle condition needs to be checked after iteration.

172

Please rename $ids to $session_log_ids.

Makes code easier to read.

181–191

Please:

  1. transform into do ... while cycle
  2. remove the $may_have_change_log

Code might look like this:

do {
	$change_log_ids = $this->Conn->GetCol($change_log_select_sql);

	if ( $change_log_ids ) {
		$change_log_temp_handler->DeleteItems('change-log', '', $change_log_ids);
	}
} while ( count($change_log_ids) == $limit );

The do ... while variation of while cycle is best to be used, when cycle condition needs to be checked after iteration.

core/units/logs/system_logs/system_logs_config.php
1 ↗(On Diff #404)

Please remove this file from diff.

File doesn't contain any relevant changes.

This revision now requires changes to proceed.Nov 9 2015, 5:19 AM
erik edited the test plan for this revision. (Show Details)Nov 9 2015, 6:59 AM
erik edited edge metadata.
erik updated this revision to Diff 406.Nov 9 2015, 7:48 AM
erik edited edge metadata.
erik edited the test plan for this revision. (Show Details)

Made requested changes for code optimization

erik edited the test plan for this revision. (Show Details)Nov 9 2015, 9:12 AM
erik edited edge metadata.
alex requested changes to this revision.Nov 9 2015, 9:15 AM
alex edited edge metadata.

The test plan seems to out of sync with reality this time as well.

  1. go to Configuration → Website → Advanced section
  2. ensure that "Keep "Session Log" for" is set to "Forever"
  3. ensure that "Track database changes to change log" is checked, save
  4. log out
  5. log in
  6. go to Tools → Query Database section
  7. launch SQL like "UPDATE inp_UserSessionLogs SET SessionEnd = -5 WHERE NOT ISNULL(SessionEnd)"

This last item would affect 0 rows in the database, because the only record in SessionLog table would be the one matching active admin session and that's the record, that isn't affected by mentioned SQL.

I recommend you to use your own test plan to test your work (on clean In-Portal install) to detect other such inconsistencies.

core/units/helpers/temp_tables_helper.php
23–38

Please:

  1. move this method inside the event handler class, where it's used
  2. remove helper class

In my initial request (create helper, that would accept ...) I've forgot to put method word. I wanted just helper method, not helper class. Sorry for confusion.

core/units/logs/session_logs/session_log_eh.php
152

Please add an empty line below this line.

To logically group connected variables together.

164

Please use inverted condition to leave the cycle (via break;) right away.

IF statement that covers 100% of the cycle scope (code inside cycle).

171–175

Do not change anything here please.

177

Please inline this variable.

Cycle condition already means I will iterate again when condition matches. There is no need to create temp variable to describe how PHP language constructs work.

183

Please inline this variable.

Cycle condition already means I will iterate again when condition matches. There is no need to create temp variable to describe how PHP language constructs work.

core/units/logs/system_logs/system_logs_config.php
1 ↗(On Diff #406)

This wasn't addressed in previous fix.

This revision now requires changes to proceed.Nov 9 2015, 9:15 AM
alex added inline comments.Nov 9 2015, 9:18 AM
core/install/cache/class_structure.php
31

The changes in these file tell, that you have some other patches applied in your working copy as well. You should Shelve them (in PhpStorm) before running the in-portal classmap:rebuild command.

erik updated this revision to Diff 407.Nov 9 2015, 9:53 AM
erik edited edge metadata.

Made requested code formatting

alex requested changes to this revision.Nov 10 2015, 4:15 AM
alex edited edge metadata.

Please thoroughly look through each comment I leave (every time on every revision). If you decide not to do as I've advised, then please explain why as a reply to relevant comment.

core/install/cache/class_structure.php
31

This wasn't addressed in previous fix.

core/units/helpers/temp_tables_helper.php
23–38

This wasn't addressed in previous fix.

core/units/logs/session_logs/session_log_eh.php
177

This wasn't addressed in previous fix.

183

This wasn't addressed in previous fix.

This revision now requires changes to proceed.Nov 10 2015, 4:15 AM
erik added inline comments.Nov 10 2015, 4:58 AM
core/units/logs/session_logs/session_log_eh.php
177

Not clear why inline this variable - it is used in three places.

183

Inline of this variable produces code style error - can't use count() function in the cycle condition.

alex added inline comments.Nov 10 2015, 5:23 AM
core/units/logs/session_logs/session_log_eh.php
177

The comment is a reminder about previous commit, which is marked with grey color, because it references previous code version. I'm talking about $change_log_need_more_iteration variable here.

183

That's ok. You still can change it. I think that this is an error in the sniff itself, that reports that line as error. I've reported it as https://github.com/squizlabs/PHP_CodeSniffer/issues/764 .

erik updated this revision to Diff 409.Nov 10 2015, 11:45 AM
erik edited edge metadata.

Removed new helper, made inline code with count() in cycle conditions

alex edited the test plan for this revision. (Show Details)Nov 11 2015, 5:01 AM
alex edited edge metadata.
alex edited the test plan for this revision. (Show Details)Nov 11 2015, 5:05 AM
alex requested changes to this revision.Nov 11 2015, 5:10 AM
alex edited edge metadata.

Code tested according to plan. Please do that last cosmetical change and we're done here.

core/units/logs/session_logs/session_log_eh.php
154–156

The SQL statement part isn't properly indented:

The SQL in the $change_log_select_sql variable below however is indented correctly:

This revision now requires changes to proceed.Nov 11 2015, 5:10 AM
erik updated this revision to Diff 410.Nov 11 2015, 6:18 AM
erik edited edge metadata.

Fixed SQL code indent

alex accepted this revision.Nov 11 2015, 6:38 AM
alex edited edge metadata.
This revision is now accepted and ready to land.Nov 11 2015, 6:38 AM
Closed by commit rINP16329: Fixes INP-1510 - Allow rotation of "Session Log" and "Changes Log" (authored by alex). · Explain WhyJan 23 2016, 2:05 PM
This revision was automatically updated to reflect the committed changes.
alex edited edge metadata.Mar 10 2016, 5:37 AM
alex added a project: Restricted Project.

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 140416300Nov 6 2015, 11:24 AM
★
★
Diff 240616300Made requested changes for code optimizationNov 9 2015, 7:48 AM
★
★
Diff 340716300Made requested code formattingNov 9 2015, 9:53 AM
★
★
Diff 440916300Removed new helper, made inline code with count() in cycle conditionsNov 10 2015, 11:45 AM
★
★
Diff 541016300Fixed SQL code indentNov 11 2015, 6:18 AM
★
★
Diff 646816328Commit rINP16329Jan 23 2016, 2:05 PM
★
★

Diff 407

View Options

core/install/cache/class_structure.php

Loading...
View Options

core/install/english.lang

Loading...
View Options

core/install/install_data.sql

Loading...
View Options

core/install/upgrades.sql

Loading...
View Options

core/units/helpers/temp_tables_helper.php

Loading...
View Options

core/units/logs/session_logs/session_log_eh.php

Loading...
View Options

core/units/logs/session_logs/session_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