Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix errors with instanceof checks for custom objects. #962

Merged
merged 5 commits into from Feb 15, 2019

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented Feb 15, 2019

Problem this Pull Request solves

@tn3rb reported a bug with building an instance of BaseEntity using the money value objects. Doing something like this:

const money = new eejs.valueObjects.Money( 0, eejs.valueObjects.SiteCurrency );
const price = await wp.data.dispatch( 'eventespresso/core' ).createEntity( 'price', { PRT_ID: 2, PRC_name: '', PRC_desc: '', PRC_amount: money, PRC_order: 0 } );

...produced the following error:

Uncaught (in promise) TypeError: Instance of Money required
    at assertMoney (money.js:22)
    at Function.assertMoney (money.js:485)
    at maybeAssertValueObject (assertions.js:41)
    at maybeConvertFromValueObjectWithAssertions (extractors.js:90)
    at isShallowValidValueForField (validators.js:128)
    at assertValidValueForPreparedField (assertions.js:154)
    at create.js:199
    at lodash.js?ver=4.17.11:4911
    at baseForOwn (lodash.js?ver=4.17.11:2996)
    at lodash.js?ver=4.17.11:4880

In debugging, there IS a money object being passed through to the assertion but its failing the instanceof check. Likely because something other than Money is on the right side of the condition in the built file.

To protect against these kinds of weird compile errors, this pull:

  • creates a new instanceOf( objectInstance, expectedName ) validator (in @eventespresso/validators) that can be used to verify a given object instance is an instance of the given name.
  • adds unit tests for the above.

I also:

  • slipped in various fixes in the entity factory entity constructor that I uncovered while testing the code snippet Brent was using.
  • fixed a redundant assertion being done in the entity factory.

How has this been tested

  • verified unit-tests pass.
  • monkey testing in the browser console.
  • Code changed impacts the EventAttendee block but it will bork immediately if there's something wrong so easily verified if it works or not.

Checklist

@tn3rb tn3rb removed their assignment Feb 15, 2019
@nerrad nerrad merged commit e965a0c into master Feb 15, 2019
@nerrad nerrad deleted the BUG/fix-for-instance-of branch February 15, 2019 12:34
eeteamcodebase pushed a commit that referenced this pull request Feb 15, 2019
…s with instanceof checks for custom objects. (#962)

## Problem this Pull Request solves
<!-- Please describe your changes in the context of the problem they solve -->

@tn3rb reported a bug with building an instance of `BaseEntity` using the money value objects.  Doing something like this:

```js
const money = new eejs.valueObjects.Money( 0, eejs.valueObjects.SiteCurrency );
const price = await wp.data.dispatch( 'eventespresso/core' ).createEntity( 'price', { PRT_ID: 2, PRC_name: '', PRC_desc: '', PRC_amount: money, PRC_order: 0 } );
```

...produced the following error:

```
Uncaught (in promise) TypeError: Instance of Money required
    at assertMoney (money.js:22)
    at Function.assertMoney (money.js:485)
    at maybeAssertValueObject (assertions.js:41)
    at maybeConvertFromValueObjectWithAssertions (extractors.js:90)
    at isShallowValidValueForField (validators.js:128)
    at assertValidValueForPreparedField (assertions.js:154)
    at create.js:199
    at lodash.js?ver=4.17.11:4911
    at baseForOwn (lodash.js?ver=4.17.11:2996)
    at lodash.js?ver=4.17.11:4880
```

In debugging, there IS a money object being passed through to the assertion but its failing the instanceof check.  Likely because something _other_ than `Money` is on the right side of the condition in the built file.

To protect against these kinds of weird compile errors, this pull:

- creates a new `instanceOf( objectInstance, expectedName )` validator (in `@eventespresso/validators`) that can be used to verify a given object instance is an instance of the given name.
- adds unit tests for the above.

I also:

- slipped in various fixes in the entity factory entity constructor that I uncovered while testing the code snippet Brent was using.
- fixed a redundant assertion being done in the entity factory.

## How has this been tested
<!-- Please describe in detail how you tested your changes and how testing can be reproduced -->
<!-- Include details of your testing environment, and tests ran to see how your changes affect other areas of code -->
<!-- Include any notes about automated tests you've written for this pull request.  Pull requests with automated tests are preferred. -->

* [x] verified unit-tests pass.
* [x] monkey testing in the browser console.
* [x] Code changed impacts the EventAttendee block but it will bork immediately if there's something wrong so easily verified if it works or not.

## Checklist

* [x] I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs
* [x] User input is adequately validated and sanitized
* [x] all publicly displayed strings are internationalized (usually using `esc_html__()`, see https://codex.wordpress.org/I18n_for_WordPress_Developers)
* [x] My code is tested.
* [x] My code follows the Event Espresso code style.
* [x] My code has proper inline documentation.
* [x] My code accounts for when the site is in Maintenance Mode (MM2 especially disallows usage of models)

"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants