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

(refactor): 4263 nnnnat non meteor schemas #4266

Merged
merged 21 commits into from
May 25, 2018

Conversation

nnnnat
Copy link
Contributor

@nnnnat nnnnat commented May 23, 2018

Resolves #4263
Impact: breaking
Type: refactor

Issue

Move/change the lib/collections/schemas files so that we can get access to schemas in devserver/Jest tests without a Meteor context.

Solution

  1. Move schemas out of lib/collections/schemas and into imports/collections/schemas
  2. Removed meteor/check and meteor/Tracker from each schema contructor.
  3. Removed shopIdAutoValue, shopIdAutoValueForCart and shopDefaultCountry autoValue functions from schemas in favor of explicitly setting those values on collection insert.
  4. Updated lib/collections/schemas/index.js to import the non-meteor schemas and add the meteor/check and meteor/Tracker to the schema's constructor for use inside the meteor application.
  5. Removed the use of shopIdAutoValue from plugin schemas and updated usage to provide a shopId on collection insert
    1. Discounts
    2. Taxes
    3. EmailTemplates
    4. Shipping-shipo
  6. Refactor server/api/core/utils.js getSlug method to just import transliteration by default and removed the autoValue functions being used in schema constructors in favor of setting the slug/handle on collection inserts.
  7. Updated schema import paths for several plugins to just import from the /lib/collections/schema/index.js and not the schema files.

Breaking changes

The schemas have moved, we're still importing them inside the lib/collections/schemas/index.js to apply the meteor/check and meteor/Tracker to the schemas when being used with meteor. I don't believe this is a breaking change necessarily but I wanted to outline it.

Aside from createdAtAutoValue and updatedAtAutoValue we're moving away from using autoValues in the schemas in favor of providing required properties. shopIdAutoValue, shopIdAutoValueForCart and shopDefaultCountry autoValue helper functions will no longer be available.

If you have custom plugins that are inserting into any collection that has a shopId property, and you rely on shopId being automatically set, you will now have to explicitly set it yourself in the inserted object.

Testing

You may need to rebuild your docker image for this branch to work

  1. Spin up a fresh RC shop, app should work as it always has.
  2. Run the app test make sure everything is passing

nnnnat added 11 commits May 22, 2018 12:53
…ons dir, removed the meteor check and tracker from the schemas, created a new lib/comllections/schemas/index.js that imports the schemas for meteor and adds the check and tracker to the schemas
…ts collections, removed the shopIdAutoValue call from the Accounts schema
@nnnnat nnnnat changed the title [WIP] (feat): 4263 nnnnat non meteor schemas [WIP] (refactor): 4263 nnnnat non meteor schemas May 23, 2018
nnnnat added 7 commits May 23, 2018 12:17
…n and not lazyLoad between the two options, updated shops and products schema to not use autovalues when creating slugs, updated fixtures and inserts to set slugs on shops and products
…s call to prevent an endless loop, updated importer to check the cbArgs in the process method and added jsdoc for the new arg
@nnnnat nnnnat requested a review from aldeed May 24, 2018 19:39
@nnnnat nnnnat changed the title [WIP] (refactor): 4263 nnnnat non meteor schemas (refactor): 4263 nnnnat non meteor schemas May 24, 2018
@nnnnat nnnnat changed the base branch from release-1.12.0 to release-2.0.0 May 24, 2018 23:30
@nnnnat nnnnat requested review from kieckhafer and removed request for aldeed May 25, 2018 16:13
*/

/**
* Reaction uses {@link https://github.com/aldeed/simple-schema-js SimpleSchema} to apply basic content and structure validation to Collections.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need both of these jsdoc blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, I believe @aldeed did this when he cleaned up the jsdoc a few months back

const product = {
title: faker.commerce.productName(),
title: productTitle,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other fields are being populated on the fly here, why move the productTitle into it's own const?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind... didn't see it was being used in the handle

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App works, able to go through the full checkout flow.

Tests pass.

Looks good!

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

Successfully merging this pull request may close these issues.

2 participants