- go to Configurartion → Website → Themes section
- confirm, that following fields are present in grid and on edit form: AddedOn, DeletedOn
- confirm, that there is no Delete toolbar button above the grid
- click on Status field search filter and confirm presence of Deleted option
- copy simple theme folder with all underlying files (but without .svn sub-folders) to the simple2 folder under the same /themes/ folder
- press Rescan Themes toolbar button
- confirm, that:
- simple2 theme appeared in the grid
- time in AddedOn column matches current moment (time, when theme was added)
- delete simple2 folder
- press Rescan Themes toolbar button
- confirm, that status of simple2 theme was changed to Deleted
- open some theme for editing
- confirm, that Name field is read-only
Details
Details
- Reviewers
alex
Diff Detail
Diff Detail
- Repository
- rINP In-Portal
- Branch
- /in-portal/branches/5.3.x
- Lint
Lint Errors Excuse: Fixing of constants file is not part of this task. - Unit
No Unit Test Coverage
Event Timeline
Comment Actions
- please use only TABs for indentation (via TeamViewer discovered, that incorrect PhpStorm settings lead to this)
- populate (in upgrade.sql) the value of AddedOn column with UNIX_TIMESTAMP()
| core/admin_templates/themes/themes_edit.tpl | ||
|---|---|---|
| 62 |
| |
| 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. | |
Comment Actions
Adding comment to create corresponding "Issue Link" from JIRA Issue back to this Differential Revision.
| core/install/upgrades.sql | ||
|---|---|---|
| 3027–3029 | increase indentation level like in other SQLs in this file | |
| core/units/helpers/themes_helper.php | ||
|---|---|---|
| 85 |
| |
Comment Actions
Renamed Themes Enabled => Status field in other units. Made requested code style fixes.
| 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. | |
| 551–557 | 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 | ||
| 526 | 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. | |
Comment Actions
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 | ||
|---|---|---|
| 3026–3029 | 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. | |
| 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 | ||
| 613 | Double check, that you've covered all usages of Enabled column and are using proper constants. Just in case. | |
| core/kernel/constants.php | ||
| 237 | Please rename into ThemeConstants. According to latest CS changes. | |
| 239–241 | Please prefix constant names with STATUS_. According to latest CS changes. | |