Page MenuHomeIn-Portal Phabricator

INP-1917 - Validate "kOptionsFormatter::Parse" method values
Needs ReviewPublic

Authored by alex on Mon, Jul 14, 12:35 PM.

Details

Reviewers
erik
Test Plan

Part 1

  1. open the /core/units/helpers/cat_dbitem_export_helper.php file for editing
  2. comment out last IF (validation error removal) in the \kCatDBItemExportHelper::setFieldValue method
  3. save changes
  4. execute tests in the kOptionsFormatterTest test case
  5. login to the Admin Console
  6. go to the Website & ContentStructure & DataProducts section
  7. create a product
  8. select that product in the grid
  9. use Export menu item under the Tools toolbar button to export at least these product info columns:
    • SKU
    • Status
    • CategoryPath
    • l1_Name
    • ProductId
    • EditorsPick
  10. open an exported CSV file for editing
  11. change Status column value to any non-existing option (e.g. Non-Existing)
  12. go to the ToolsImport Data section
  13. import that modified CSV file
  14. go to the Website & ContentStructure & DataProducts section
  15. confirm that a new product wasn't created as a result of the CSV import
  16. go to the Logs & ReportsSystem Log section
  17. confirm, that:
    • there is a record about failed product creation due to the unknown_option validation error
    • it shows the wrong option title (from the CSV file)

Part 2

  1. login to the Admin Console
  2. go to the Website & ContentPolls section
  3. create a poll in Active status
  4. open that poll for editing
  5. in the poll unit config comment-out Active option
  6. in Admin Console try saving that poll
  7. confirm that you'll get a nice error back saying, that 1 option doesn't exist

Part 3

IMPORTANT: Do this from the Web Browser by hand to test that human can do this.
  1. locate all units that use kOptionsFormatter formatter
  2. for each unit (core - 40 units; custom - 1 unit; in-bulletin - 8 units; in-commerce - 27 units; in-link - 4 units; in-news - 1 unit):
    1. try adding a new record
    2. try editing and changing the above-added record
    3. confirm that the unknown_option validation error (introduced in this Differential Revision) isn't happening
    4. report back each unit where an error happened during correct usage and explain that usage

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.2.x
Lint
Lint ErrorsExcuse: Unrelated
SeverityLocationCodeMessage
Errorcore/kernel/utility/formatters/options_formatter.php:166PHPCS.E.Squiz.Strings.DoubleQuoteUsage.ContainsVarSquiz.Strings.DoubleQuoteUsage.ContainsVar
Errorcore/kernel/utility/formatters/options_formatter.php:166PHPCS.E.Squiz.Strings.DoubleQuoteUsage.ContainsVarSquiz.Strings.DoubleQuoteUsage.ContainsVar
Errorcore/kernel/utility/validator.php:62PHPCS.E.CodingStandard.Formatting.ItemAssignment.MixedWhitespaceAfterCodingStandard.Formatting.ItemAssignment.MixedWhitespaceAfter
Errorcore/kernel/utility/validator.php:62PHPCS.E.CodingStandard.Formatting.ItemAssignment.MixedWhitespaceBeforeCodingStandard.Formatting.ItemAssignment.MixedWhitespaceBefore
Errorcore/kernel/utility/validator.php:62PHPCS.E.Squiz.WhiteSpace.OperatorSpacing.SpacingAfterSquiz.WhiteSpace.OperatorSpacing.SpacingAfter
Errorcore/kernel/utility/validator.php:63PHPCS.E.CodingStandard.Commenting.InlineComment.InvalidEndCharCodingStandard.Commenting.InlineComment.InvalidEndChar
Errorcore/kernel/utility/validator.php:63PHPCS.E.CodingStandard.Formatting.ItemAssignment.MixedWhitespaceAfterCodingStandard.Formatting.ItemAssignment.MixedWhitespaceAfter
Errorcore/kernel/utility/validator.php:63PHPCS.E.CodingStandard.Formatting.ItemAssignment.MixedWhitespaceBeforeCodingStandard.Formatting.ItemAssignment.MixedWhitespaceBefore
Errorcore/kernel/utility/validator.php:63PHPCS.E.CodingStandard.Formatting.SpaceOperator.SpacingBeforeCodingStandard.Formatting.SpaceOperator.SpacingBefore
Errorcore/kernel/utility/validator.php:63PHPCS.E.Squiz.WhiteSpace.OperatorSpacing.SpacingAfterSquiz.WhiteSpace.OperatorSpacing.SpacingAfter
Errorcore/tests/Unit/kernel/utility/formatters/kOptionsFormatterTest.php:4PHPCS.E.Squiz.Classes.ValidClassName.NotCamelCapsSquiz.Classes.ValidClassName.NotCamelCaps
Unit
No Unit Test Coverage
Build Status
Buildable 11099
Build 3799: arc lint + arc unit

Event Timeline

alex created this revision.Mon, Jul 14, 12:35 PM
alex requested review of this revision.Mon, Jul 14, 12:35 PM
alex updated this revision to Diff 1354.Mon, Jul 14, 12:39 PM

Added missing phrase.

alex edited the test plan for this revision. (Show Details)Mon, Jul 14, 12:43 PM
alex edited the test plan for this revision. (Show Details)Tue, Jul 15, 2:17 AM
alex edited the test plan for this revision. (Show Details)Fri, Jul 18, 12:56 PM
alex edited the test plan for this revision. (Show Details)Sat, Jul 19, 5:44 AM
erik added a comment.Thu, Jul 24, 1:51 PM

Made partial testing. In the u and addr units, on State field Detected unknown_option validation error. As discussed in Zoom, started writing code for automated unit test to check create/update all units with kOptionsFormatter.

alex added a comment.Fri, Jul 25, 7:46 AM
In D529#10843, @erik wrote:

Made partial testing. In the u and addr units, on State field Detected unknown_option validation error. As discussed in Zoom, started writing code for automated unit test to check create/update all units with kOptionsFormatter.

Instead of writing a unit test for each unit by hand you can automate this like so:

  1. create a test case file in each module
  2. in that test case file create a single test with a data provider
  3. data provider would get unit config prefixes of the module (even core), where that unit belongs
  4. test itself would accept unit config prefix and:
    • create an empty object (with skip_autoload) of that unit
    • fill required fields with any random stuff (use \kCatDBItemExportHelper::fillRequiredFields method to understand how to do this)
    • attempt to persist that object via kDBItem::Create method
    • assert, that no validation errors array is empty (this way you'll see validation errors in the test suite output)

.

erik added a comment.Mon, Jul 28, 11:46 AM

I do not understand relation between test plan and proposed automation logic.

  1. As I understand from test plan, not only required fields should be tested, but fields with specific formatter.
  2. As I understand from test plan, should test not only create, but also update.
  3. Each unit may be specific, may have dependencies to another units, so proposed automation logic seems is not helpful.
alex added a comment.Tue, Jul 29, 2:21 AM
In D529#10861, @erik wrote:

I do not understand relation between test plan and proposed automation logic.

  1. As I understand from test plan, not only required fields should be tested, but fields with specific formatter.
  2. As I understand from test plan, should test not only create, but also update.
  3. Each unit may be specific, may have dependencies to another units, so proposed automation logic seems is not helpful.

Try it and see how it goes. If it helps to save time, then it was already worth it. For the complex units, where it won't work, you can create individual tests.

erik added a comment.Thu, Jul 31, 12:10 PM

Partially tested (done testing for Core module part).

  1. Languages.

1.1. Broken In-Portal clean install due error in lang prefix, preventing language record creation, required in normal installation process.
1.2. Due specific option_sql configuration for PackName and LocalName fields, possible add only record with values, that already exists. So, when no records in Languages table - can't add first language record.

  1. Categories - works well in admin interface, but problematic to add through object, because of Priority field options, that are populated only in OnPreSaveCreated event hook.
  2. Change logs - sometimes can't create through object same as languages, due specific Prefix and MasterPrefix field options_sql configuration. But in system records are created by direct SQL, so this not a problem. maybe.
  3. Users - can't create and update records with US State, because state options are populated too late - after SetFieldsFromHash already set error to the State field.
erik requested changes to this revision.Tue, Aug 5, 12:27 PM

Tested all units, additional bugs found in the In-Commerce module.

  1. Manufacturers - can't create and update records with US State, because state options are populated too late - after SetFieldsFromHash already set error to the State field.
  2. User addresses - can't create and update records with US State, because state options are populated too late - after SetFieldsFromHash already set error to the State field.
  3. Orders - can't update shipping and billing address from non-USA Country to USA - error "unknown option" on BillingState abd ShippingState fields.

Used init testing classes -

This revision now requires changes to proceed.Tue, Aug 5, 12:27 PM
alex updated this revision to Diff 1371.Wed, Aug 6, 9:15 AM

Added more unit tests (original version created by Erik).

alex added a comment.Wed, Aug 6, 9:21 AM

Found these problems and fixed them (related to tests in the patch):

  1. test classes in different modules are named the same, which would result in a fatal error (unable to redeclare a test case class), when executed together
  2. test case files aren't placed in the Unit sub folder
  3. non-efficient performance-wise:
    • the same user object is created before & deleted after every test
    • the same category objects are loaded before every test
  4. identical (at least in values that use the "unique" option in the unit config) base items were created in different tests, and that created issues (failed to create base item), when tests are executed in parallel
  5. no protection against failure at base item creation

Found these problems, but wasn't able to fix them:

  • missing the test cases for core and modules/custom folders