Unexpected output: Affected paths: relative - resubmit patch.
⚙ D12 INP-1312 - Mark themes as deleted instead of actually deleting them
Page MenuHomeIn-Portal Phabricator
Differential D12

INP-1312 - Mark themes as deleted instead of actually deleting them
Needs ReviewPublic
Actions

Authored by erik on Sep 25 2014, 10:35 AM.

Details

Reviewers
alex
Test Plan
  1. go to Configurartion → Website → Themes section
  2. confirm, that following fields are present in grid and on edit form: AddedOn, DeletedOn
  3. confirm, that there is no Delete toolbar button above the grid
  4. click on Status field search filter and confirm presence of Deleted option
  5. copy simple theme folder with all underlying files (but without .svn sub-folders) to the simple2 folder under the same /themes/ folder
  6. press Rescan Themes toolbar button
  7. confirm, that:
    • simple2 theme appeared in the grid
    • time in AddedOn column matches current moment (time, when theme was added)
  8. delete simple2 folder
  9. press Rescan Themes toolbar button
  10. confirm, that status of simple2 theme was changed to Deleted
  11. open some theme for editing
  12. confirm, that Name field is read-only

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.3.x
Lint
Lint ErrorsExcuse: Full files re-formatting is not part of this task.
SeverityLocationCodeMessage
Errorcore/kernel/constants.php:237PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Errorcore/kernel/constants.php:237PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/kernel/constants.php:239PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeKeywordCodingStandard.Classes.ClassDeclaration.SpaceBeforeKeyword
Errorcore/kernel/constants.php:239PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/kernel/constants.php:240PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeBrace
Errorcore/kernel/constants.php:244PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Errorcore/kernel/constants.php:244PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:73PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:83PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:86PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:87PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:90PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:95PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:96PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:132PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:624PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/themes/themes_config.php:2PHPCS.E.CodingStandard.Commenting.DocComment.MissingShortCodingStandard.Commenting.DocComment.MissingShort
Errorcore/units/themes/themes_config.php:3PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/themes/themes_config.php:4PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/themes/themes_config.php:44PHPCS.E.CodingStandard.Array.Array.SpaceAfterKeywordCodingStandard.Array.Array.SpaceAfterKeyword
Errorcore/units/themes/themes_config.php:44PHPCS.E.Generic.PHP.LowerCaseKeyword.FoundGeneric.PHP.LowerCaseKeyword.Found
Errorcore/units/themes/themes_config.php:61PHPCS.E.CodingStandard.Array.Array.SpaceAfterKeywordCodingStandard.Array.Array.SpaceAfterKeyword
Errorcore/units/themes/themes_config.php:61PHPCS.E.Generic.PHP.LowerCaseKeyword.FoundGeneric.PHP.LowerCaseKeyword.Found
Errorcore/units/themes/themes_config.php:61PHPCS.E.Squiz.Strings.DoubleQuoteUsage.NotRequiredSquiz.Strings.DoubleQuoteUsage.NotRequired
Errorcore/units/themes/themes_config.php:141PHPCS.E.Generic.PHP.LowerCaseKeyword.FoundGeneric.PHP.LowerCaseKeyword.Found
Unit
No Unit Test Coverage
Build Status
Buildable 781
Build 781: arc lint + arc unit

Event Timeline

erik updated this revision to Diff 17.Sep 25 2014, 10:35 AM
erik retitled this revision from to Improving theme deleting alghoritm.
erik updated this object.
erik edited the test plan for this revision. (Show Details)
erik added a reviewer: alex.
erik set the repository for this revision to rINP In-Portal.
erik added 1 JIRA issue(s): INP-1312.
alex requested changes to this revision.Sep 25 2014, 10:50 AM
alex edited edge metadata.
  1. please use only TABs for indentation (via TeamViewer discovered, that incorrect PhpStorm settings lead to this)
  2. populate (in upgrade.sql) the value of AddedOn column with UNIX_TIMESTAMP()
core/admin_templates/themes/themes_edit.tpl
62
  • rename Enabled column to Status, since there can't be 3 ways how theme can be enabled
  • remove title attribute
core/units/helpers/themes_helper.php
73–74

Please use named constants instead of magic numbers, e.g. 2 (for Deleted) and 0 (for Disabled). Same goes in other changed files.

This revision now requires changes to proceed.Sep 25 2014, 10:50 AM
alex added a comment.Nov 23 2014, 3:12 AM

Adding comment to create corresponding "Issue Link" from JIRA Issue back to this Differential Revision.

alex retitled this revision from Improving theme deleting alghoritm to INP-1312 - Improving theme deleting alghoritm.Dec 7 2014, 3:31 PM
alex edited edge metadata.
alex set the repository for this revision to rINP In-Portal.Dec 7 2014, 3:41 PM
erik edited the test plan for this revision. (Show Details)Mar 23 2015, 10:52 AM
alex edited the test plan for this revision. (Show Details)Mar 23 2015, 12:21 PM
erik updated this revision to Diff 145.Mar 25 2015, 5:47 AM
erik edited edge metadata.

Changes in theme adding/deleting processes, related DB structure changes.

alex requested changes to this revision.May 19 2015, 9:53 PM
alex edited edge metadata.
alex added inline comments.
core/install/upgrades.sql
3042–3044

increase indentation level like in other SQLs in this file

This revision now requires changes to proceed.May 19 2015, 9:53 PM
erik updated this revision to Diff 194.May 20 2015, 12:17 PM
erik edited edge metadata.

Made standard offset for secondary rows of sql command.

alex requested changes to this revision.May 20 2015, 2:55 PM
alex edited edge metadata.
alex added inline comments.
core/units/helpers/themes_helper.php
85
  1. rename variable to $theme_disabled
  2. check for disabled status specifically, e.g. $theme_info['Status'] == ThemeStatus::DISABLED
  3. replace all usages of $theme_enabled (e.g. invert condition there)
This revision now requires changes to proceed.May 20 2015, 2:55 PM
erik updated this revision to Diff 201.May 26 2015, 4:36 AM
erik edited edge metadata.

Renamed Themes Enabled => Status field in other units. Made requested code style fixes.

alex requested changes to this revision.May 31 2015, 4:36 AM
alex edited edge metadata.
alex added inline comments.
core/install.php
912

Please also use ThemeStatus class constants here.

core/install/cache/class_structure.php
39

Apparently you were having changes from other task present as well, when building this patch. Please stash/revert changes to files non-relevant to the patch before running in-portal classmap:rebuild command.

552–558

Apparently you were having changes from other task present as well, when building this patch. Please stash/revert changes to files non-relevant to the patch before running in-portal classmap:rebuild command.

core/kernel/application.php
528
NOTE: Apply similar fix in other relevant places.

Since now we have 3 statuses, then !$theme->GetDBField('Status') comparison can't be made anywhere. Instead we should be exactly checking for what's needed.

In this case we should be doing $theme->GetDBField('Status') != ThemeStatus::ACTIVE.

core/units/helpers/themes_helper.php
79
NOTE: Fix in other similar places.

According to http://php.net/manual/en/function.mktime.php since PHP 5.1 usage of mktime() without arguments would result in E_STRICT warning.

Please use time() function instead.

572

Please also use ThemeStatus class constants here.

core/units/themes/themes_config.php
159

For existing In-Portal installations this change would result in Status column be added to the end of the grid.

Please add corresponding DELETE FROM UserPersistentSessionData ... SQL statement to reset user grid column preferences.

This revision now requires changes to proceed.May 31 2015, 4:36 AM
erik updated this revision to Diff 257.Aug 18 2015, 4:43 AM
erik edited edge metadata.

Different reguested fixes + added missing phrase.

alex retitled this revision from INP-1312 - Improving theme deleting alghoritm to INP-1312 - Mark themes as deleted instead of actually deleting them.Jul 15 2016, 2:39 PM
alex edited edge metadata.
alex requested changes to this revision.Nov 21 2016, 3:15 AM
alex edited edge metadata.

Also test plan doesn't have regression testing part, where all changed code is invoked and confirmed, that it works as before (places where you now do Status = ThemeStatus::ACTIVE instead of Enabled = 1.

core/install/upgrades.sql
3041–3044

Please set value of AddedOn column for existing themes to current moment (the UNIX_TIMESTAMP() function result if I'm not mistaken).

Themes existed before/after this patch will have inconsistent values in AddedOn field.

This revision now requires changes to proceed.Nov 21 2016, 3:15 AM
erik updated this revision to Diff 726.Apr 14 2017, 6:00 AM
erik edited edge metadata.

Set AddedOn values for themes on install/upgrade.

erik updated this revision to Diff 727.Apr 14 2017, 6:37 AM
erik edited edge metadata.

Filled AddedOn date in themes table.

alex requested changes to this revision.Apr 14 2017, 6:41 AM
alex edited edge metadata.
alex added inline comments.
core/install/install_schema.sql
376

Please revert change to a column name.

BC break that I wasn't aware of when suggested to rename the column in first place. Sorry.

core/kernel/application.php
615

Double check, that you've covered all usages of Enabled column and are using proper constants.

Just in case.

core/kernel/constants.php
239

Please rename into ThemeConstants.

According to latest CS changes.

241–243

Please prefix constant names with STATUS_.

According to latest CS changes.

This revision now requires changes to proceed.Apr 14 2017, 6:41 AM
erik added a comment.Jun 28 2017, 5:58 AM

Today made locally all requested changes, left testing by plan.

erik updated this revision to Diff 788.Jul 18 2017, 5:46 AM
erik edited edge metadata.

Reverted change of Enabled field name.

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 117Sep 25 2014, 10:34 AM
★
★
Diff 214516135Changes in theme adding/deleting processes, related DB structure changes.Mar 25 2015, 5:47 AM
★
★
Diff 319416195Made standard offset for secondary rows of sql command.May 20 2015, 12:17 PM
★
★
Diff 420116195Renamed Themes Enabled => Status field in other units. Made requested code style fixes.May 26 2015, 4:36 AM
★
★
Diff 525716207Different reguested fixes + added missing phrase.Aug 18 2015, 4:43 AM
★
★
Diff 672616543Set AddedOn values for themes on install/upgrade.Apr 14 2017, 6:00 AM
★
★
Diff 772716543Filled AddedOn date in themes table.Apr 14 2017, 6:37 AM
★
★
Diff 878816551Reverted change of Enabled field name.Jul 18 2017, 5:46 AM
★
★

Diff 788

View Options

core/admin_templates/themes/themes_edit.tpl

Loading...
View Options

core/admin_templates/themes/themes_list.tpl

Loading...
View Options

core/install.php

Loading...
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/install_schema.sql

Loading...
View Options

core/install/upgrades.sql

Loading...
View Options

core/kernel/application.php

Loading...
View Options

core/kernel/constants.php

Loading...
View Options

core/kernel/managers/rewrite_url_processor.php

Loading...
View Options

core/kernel/nparser/compiler.php

Loading...
View Options

core/units/admin/admin_events_handler.php

Loading...
View Options

core/units/categories/categories_config.php

Loading...
View Options

core/units/categories/categories_event_handler.php

Loading...
View Options

core/units/helpers/themes_helper.php

Loading...
View Options

core/units/structure/structure_config.php

Loading...
View Options

core/units/themes/themes_config.php

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