Unexpected output: Affected paths: relative - resubmit patch.
⚙ D256 INP-1325 - Perform intellectual cache update on category changes
Page MenuHomeIn-Portal Phabricator
Differential D256

INP-1325 - Perform intellectual cache update on category changes
Needs RevisionPublic
Actions

Authored by alex on Nov 2 2016, 2:50 PM.

Details

Reviewers
erik
Test Plan

Terminology:

  • leaf category - category without sub-categories inside it
  • sub-tree - category with sub-categories inside it

Recycle Bin functionality

  • to enable:
    1. create a Recycle Bin category (name doesn't matter)
    2. specify it's ID in the RecycleBinFolder system setting in Configuration → Website → Output section
  • to disable:
    1. clear value of the RecycleBinFolder system setting in Configuration → Website → Output section

Fields to be checked after each performed action in Categories table:

  • ParentPath
  • NamedParentPath
  • NamedParentPathHash
  • CachedTemplate
  • CachedTemplateHash
  • CachedDescendantCatsQty
  • TreeLeft
  • TreeRight

Actions to be tested:

  • adding new leaf category
  • deleting leaf category with Recycle Bin functionality disabled
  • deleting sub-tree with Recycle Bin functionality disabled
  • NEW: deleting leaf category with Recycle Bin functionality enabled
  • NEW: deleting sub-tree with Recycle Bin functionality enabled
  • moving leaf category via Cut and Paste buttons on toolbar
  • moving sub-tree via Cut and Paste buttons on toolbar
  • moving leaf category by changing Parent Section dropdown during category editing
  • moving sub-tree by changing Parent Section dropdown during sub-tree's root category editing
  • NEW: changing these fields of leaf: Filename, Template
  • NEW: changing these fields of sub-tree: Filename, Template

Test goals

  1. above specified fields (except TreeLeft and TreeRight) will have exactly same values for ALL categories, when category is added/moved/deleted compared to field values after full category cache rebuild (done via Tools > Rebuild Section Cache action)
  2. the TreeLeft and TreeRight fields will have equivalent values (actual values might be different, but doing TreeLeft BETWEEN A AND B sql will yield same set of categories) for ALL categories, when category is added/moved/deleted compared to field values after full category cache rebuild (done via Tools > Rebuild Section Cache action)
  3. after any performed action the + icon before category name in category tree in left frame of Admin Console must be shown only for categories, that have sub-categories (sub-tree)
  4. after any performed action the CachedDescendantCatsQty of a category should tell how much categories there are in a sub-tree (on all levels)
  5. the menu cache, used by <inp2:st_CachedMenu tag is reset after any of above hierarchy changing actions
  6. the category list cache in Parent Section dropdown is reset after any of above hierarchy changing actions

Test Plan (part 1)

  • perform each action from above list and confirm, that it does meet all test goals listed above

Test Plan (part 2)

  • perform clean install of In-Portal 5.2.x with Patch applied
  • confirm, that value of Section Permission Rebuild Mode system setting in Configuration → Website → Output section is set to Manual
  • perform clean install of In-Portal 5.2.1
  • confirm, that value of Section Permission Rebuild Mode system setting in Configuration → Website → Output section is set to Automatic
  • perform upgrade to In-Portal 5.2.x with Patch applied
  • confirm, that value of Section Permission Rebuild Mode system setting in Configuration → Website → Output section is set to Manual

Test Plan (part 3)

  • go to Website & Content → Structure & Data section
  • click on Sections tab
  • click on Tools button on toolbar and select Edit Current Section action
  • change some permissions in there
  • save changes
  • confirm, that changes were saved and no error happened by opening same category editing

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.2.x
Lint
Lint ErrorsExcuse: Won't be fixing in 5.2.x.
SeverityLocationCodeMessage
Errorcore/units/categories/cache_updater.php:396PHPCS.E.Generic.Files.LineLength.MaxExceededGeneric.Files.LineLength.MaxExceeded
Errorcore/units/categories/cache_updater.php:405PHPCS.E.CodingStandard.Formatting.SpaceOperator.SpacingBeforeCodingStandard.Formatting.SpaceOperator.SpacingBefore
Errorcore/units/categories/cache_updater.php:405PHPCS.E.Squiz.WhiteSpace.OperatorSpacing.SpacingAfterSquiz.WhiteSpace.OperatorSpacing.SpacingAfter
Errorcore/units/categories/cache_updater.php:573PHPCS.E.CodingStandard.Commenting.FunctionComment.MissingCodingStandard.Commenting.FunctionComment.Missing
Errorcore/units/categories/cache_updater.php:573PHPCS.E.CodingStandard.NamingConventions.ValidFunctionName.NotCamelCapsCodingStandard.NamingConventions.ValidFunctionName.NotCamelCaps
Errorcore/units/categories/cache_updater.php:573PHPCS.E.Squiz.Scope.MethodScope.MissingSquiz.Scope.MethodScope.Missing
Errorcore/units/categories/cache_updater.php:664PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Errorcore/units/categories/categories_item.php:18PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeBrace
Errorcore/units/categories/categories_item.php:297PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Errorcore/units/helpers/category_helper.php:378PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Errorcore/units/helpers/recursive_helper.php:234PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Unit
No Unit Test Coverage
Build Status
Buildable 478
Build 478: arc lint + arc unit

Event Timeline

alex updated this revision to Diff 608.Nov 2 2016, 2:50 PM
alex retitled this revision from to INP-1325 - Perform intellectual cache update on category changes.
alex updated this object.
alex edited the test plan for this revision. (Show Details)
alex added 1 JIRA issue(s): INP-1325.
alex edited the test plan for this revision. (Show Details)Nov 2 2016, 3:31 PM
alex edited edge metadata.
alex added a project: Restricted Project.
alex edited the test plan for this revision. (Show Details)
alex edited the test plan for this revision. (Show Details)Nov 2 2016, 3:33 PM
alex edited the test plan for this revision. (Show Details)Nov 4 2016, 6:56 AM
alex edited the test plan for this revision. (Show Details)
erik requested changes to this revision.EditedNov 10 2016, 5:20 AM
erik edited edge metadata.

Bug detected - after adding leaf, and then another leaf under previous leaf, we got situation when subchildren TreeLeft is not between grandparent's TreeLeft and TreeRight

This revision now requires changes to proceed.Nov 10 2016, 5:20 AM
alex added a comment.Nov 10 2016, 7:32 AM
In D256#4860, @erik wrote:

Bug detected - after adding leaf, and then another leaf under previous leaf, we got situation when subchildren TreeLeft is not between grandparent's TreeLeft and TreeRight

Based on above screenshot you've:

  1. create C_1_3_1_2_1_1 category
  2. inside it create C_BUG category
  3. all categories in parent path of C_BUG category will have incorrect TreeLeft/TreeRight

?

erik added a comment.Nov 10 2016, 7:40 AM

Typical test SQL is mentioned in the test plan. That is - TreeLeft BETWEEN A AND B. On screenshot with red are marked values A, B and TreeLeft, which results in wrong result. Red-marked TreeLeft is in C_BUG record, it is not between A and B, marked with red in grandparent of C_BUG record.

alex added a comment.Nov 10 2016, 8:15 AM
In D256#4865, @erik wrote:

Typical test SQL is mentioned in the test plan. That is - TreeLeft BETWEEN A AND B. On screenshot with red are marked values A, B and TreeLeft, which results in wrong result. Red-marked TreeLeft is in C_BUG record, it is not between A and B, marked with red in grandparent of C_BUG record.

After talking over Skype the answer to my previous question is Yes.

alex edited the test plan for this revision. (Show Details)Nov 14 2016, 5:48 AM
alex edited edge metadata.
alex edited the test plan for this revision. (Show Details)Nov 14 2016, 7:53 AM
alex added a comment.Nov 14 2016, 8:03 AM

Detected another problem, where change in Filename and Template fields of a category wasn't causing cached fields build from above fields to be updated in it's sub-categories.

alex added a comment.Nov 14 2016, 8:13 AM
In D256#4860, @erik wrote:

Bug detected - after adding leaf, and then another leaf under previous leaf, we got situation when subchildren TreeLeft is not between grandparent's TreeLeft and TreeRight

In images below L: shows TreeLeft value and R: shows TreeRight value).

Unable to reproduce locally. Here is what I did on clean install:

  1. category state after full cache rebuild:
  2. category state after adding Sub Directory category inside Directory category:
  3. category state after adding Sub Sub Directory category inside Directory > Sub Directory category:
alex updated this revision to Diff 611.Nov 14 2016, 8:17 AM
alex edited edge metadata.

Fixes case, when cached fields populated from Filename and Template field of parent category were not updated when parent category Filename and Template fields were changed.

alex planned changes to this revision.Nov 14 2016, 8:21 AM
In D256#4878, @alex wrote:
In D256#4860, @erik wrote:

Bug detected - after adding leaf, and then another leaf under previous leaf, we got situation when subchildren TreeLeft is not between grandparent's TreeLeft and TreeRight

In images below L: shows TreeLeft value and R: shows TreeRight value).

Unable to reproduce locally. Here is what I did on clean install:

  1. category state after full cache rebuild:
  2. category state after adding Sub Directory category inside Directory category:
  3. category state after adding Sub Sub Directory category inside Directory > Sub Directory category:

Ups. I can see now, that there is a problem indeed. Reopening for fixing.

alex updated this revision to Diff 612.Nov 14 2016, 1:09 PM
alex edited edge metadata.

Fixing issue, where TreeLeft and TreeRight columns weren't updated, when new category was added.

alex edited the test plan for this revision. (Show Details)Nov 21 2016, 4:07 AM
alex edited the test plan for this revision. (Show Details)Nov 21 2016, 4:09 AM
erik added a comment.Nov 21 2016, 8:56 AM

Today partially tested part of part 1. Only actions for leaf operation with enabled "Recycle Bin". No bugs found.

erik requested changes to this revision.EditedNov 24 2016, 5:37 AM
erik edited edge metadata.

Detected bug, when one admin user opens to edit category1 with relatively big TreeLeft/TreeRight numbers. Then second admin user moves subtree, where category1 is a leaf to the zone of smaller TreeLeft/TreeTight numbers. Then, first user saves category1 and old, big TreeLeft/TreeRight numbers becomes saved to the zone of small TreeLeft/TreeRight numbers, breaking Treeleft BETWEEN A AND B logic.

Before:


Middle (subtree is correctly moved):

After (category is incorrectly saved):

This revision now requires changes to proceed.Nov 24 2016, 5:37 AM
alex added a comment.Jan 19 2017, 12:16 PM

Fixing plan:

  1. add CategoryHelper::calculateTreeHash method, that will:
    • run SELECT CONCAT(CategoryId, ':', TreeLeft, '-', TreeRight) FROM Categories ORDER BY CategoryId ASC sql
    • return checksum from concatenating all retrieved records
  2. in c:OnPreCreate and c:OnEdit events store result of calling CategoryHelper::calculateTreeHash method in the category_editing_tree_hash_{window_id} session variable
  3. in c:OnSave event compare result of calling CategoryHelper::calculateTreeHash method with previously stored result (from c:OnPreCreate and c:OnEdit events) and if they don't match set CategoryPermissionRebuildModeOverride session variable to CategoryPermissionRebuild::AUTOMATIC constant value

That should block quick category cache rebuild in cases, when category tree state was changed while a category was in editing process.

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 160816388Nov 2 2016, 2:50 PM
★
★
Diff 261116428Fixes case, when cached fields populated from `Filename` and `Template` field of parent category were not updated when parent category `Filename` and `Template` fields were changed.Nov 14 2016, 8:17 AM
★
★
Diff 361216428Fixing issue, where `TreeLeft` and `TreeRight` columns weren't updated, when new category was added.Nov 14 2016, 1:09 PM
★
★

Diff 608

View Options

core/install/install_data.sql

Loading...
View Options

core/install/install_schema.sql

Loading...
View Options

core/install/upgrades.sql

Loading...
View Options

core/units/admin/admin_tag_processor.php

Loading...
View Options

core/units/categories/cache_updater.php

Loading...
View Options

core/units/categories/categories_event_handler.php

Loading...
View Options

core/units/categories/categories_item.php

Loading...
View Options

core/units/categories/categories_tag_processor.php

Loading...
View Options

core/units/helpers/category_helper.php

Loading...
View Options

core/units/helpers/recursive_helper.php

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