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): 3662 customer product grid publishing #3876

Merged
merged 89 commits into from Mar 13, 2018

Conversation

6 participants
@nnnnat
Copy link
Member

nnnnat commented Mar 1, 2018

Resolves #3662
Impact: breaking
Type: performance|refactor

Issue

The customer view of the product grid was taking an incredibly long time to load or in many cases just crashing the browser tab.

Several issues with the product grid components that were causing slow down:

  • grid items had deep and confusing nested props.
  • grid items were using inherited prop callback to get values already in the component's props.
  • grid items had a lot of conditional logic only needed for admin views.
  • the product grid was destructuring all of its props onto each grid item, this was causing each grid item to have not only it's own product data but the entire array of products as well.

The Products publication also had a lot of code that is only needed for the admin views dealing with editing and revisions of products.

Solution

@mikemurray and I have been approaching this issue by separating the customer view from the admin view.

  • Created a new "Catalog" collection that doesn't worry about any admin needed data, attaches product variants to the top-level product, and fetches the appropriate product media.
  • Created a new customer's "Products/grid" publication that pulls from the new Catalog collection.
  • Created a few React components that only deal with customer interaction.

Breaking changes

The old imports/plugins/included/product-variants/containers/productsContainer.js has been renamed to productsContainerAdmin.js and a new component named productsContainer.js now handles which products container to load based on the user's permissions.

Testing

You'll need this PR version of reaction-devtool installed to test since this PR is branched from the CFS updates

Keep the RC shop open in two browsers, one logged in as an admin the other viewing not logged in as a customer

Testing customer view loading

  1. Start up a fresh RC shop and log in as an Admin.
  2. Open devtools panel and load the medium data set. Once that finishes loading, you'll need to bulk publish the new products. Scroll to the bottom for the product grid and click "Load More" a few times so you'll have a large product list loaded then bulk publish them by clicking the Publish button. (the devtools currently will not publish the created products to the Catalog collection so we need to publish them from the admin view)
  3. In the other browser, view the RC shop as a customer to see product grid loading the large data set, scroll to the bottom of the page to load in more products.
  4. Follow acceptance tests 1A + 1B to verify customer views still working.

Testing inventory badges

  1. As an admin create a new product with 0 inventory and publish.
  2. In the other browser viewing as a customer, you should see the product in the grid with a "Sold Out" badge.
  3. As an admin turn on back order for the sold-out product.
  4. In the other browser as a customer, you should see the product with a "backorder" badge.
  5. As an admin change the product's quantity to be 3 and the warning quantity to be 5.
  6. In the other browser as a customer, you should see the product with a "Limited Supply" badge.

Testing restocking of inventory / canceling orders
Updating the inventory on an order cancel has been fixed in this PR, but has yet to be merged into this branch. This test will have to wait until we've merged with release v1.8.2

  1. As an admin create a new product with a quantity of 1 and publish it.
  2. In the other browser as a customer you should see the new product appear in the product grid, with a "Limited Supply" badge.
  3. As an admin turn off edit mode, select the new product and complete the checkout process to purchase the item.
  4. As an admin now go to process the new order, select the order to open the settings panel and click the "approve" button.
  5. In the other browser as a customer the product should have updated to show ether the "backorder" or "Sold Out!" badges.
  6. As an admin in the order panel now click the "capture payment" button.
  7. Once the payment capture is finished you should see a "cancel order" button, click that button then follow the promps and refund & restock the order by clicking the "Yes, and restock" button.
  8. In the other browser as a customer the product should have updated to show the "Limited Supply" badge.

Testing images & layout

  1. As an admin select a product and add 3-4 images.
  2. In the other browser viewing as a customer, the product should have updated with the new images.
  3. As an admin select that same product in the product and change it's layout to the "medium" layout.
  4. In the other browser viewing as a customer, the product layout should have changed and the additional images should be visible.
  5. As an admin delete one of the images from the product.
  6. In the other browser as a customer, the product images should have updated to remove the deleted image.

Testing filtering

  1. As an admin reset and load a medium data set from the reaction-devtools.
  2. Follow the steps above to bulk publish the products to the Catalog.
  3. In the other browser as a customer, add a price query ?price.min=30 to the URL
  4. The product grid should filter based on the price.

Notes

  • Canceling an order will not restock the inventory until we've merged with RC 1.8.2
  • There are some know issues with the marketplace
  • Issue for the "Catalog" collection #3877
  • Simple schema needs to be added for the Catalog collection
  • reaction-devtools needs to be updated to handle publishing to the catalog

nnnnat and others added some commits Feb 28, 2018

refactor: WIP creating a new product grid publication, updated produc…
…tsContainer to sub to the new product grid publication
refactor: cleaning up code, moved temp product image into renderMedia…
… method, added getter for the product url
fix: updated media path inside of the customer grid publication, upda…
…ted the media return to have all avalible image crops
refactor: cleaning up gridItem component, added pinned and weight cla…
…sses, updated prodCridContainer to check if posiiton is there before passing it down as a prop
refactor: added proptypes to productsContainer, updated producstConta…
…inerAdmin to use old products publication
refactor: updated product-variant index to import the new ProductsCon…
…tainer and the renamed ProductsContainerAdmin

@nnnnat nnnnat added this to the Release 1.9 milestone Mar 1, 2018

renderMedia() {
const { product } = this.props;
const MEDIA_PLACEHOLDER = "/resources/placeholder.gif";
const { large } = (Array.isArray(product.media) && product.media[0]) || { large: MEDIA_PLACEHOLDER };

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Mar 9, 2018

Member

large seems like a strange const name. Not a big deal at all, just seemed odd reading through it. Why not just media or img?

This comment has been minimized.

Copy link
@mikemurray

mikemurray Mar 9, 2018

Member

large is one of the sizes of the media object. small, medium, 'large, 'image', thumbnail`

data-id={product._id}
id={product._id}
>
<div className={(isSearch) ? "item-content" : ""}>

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Mar 9, 2018

Member

Why do we allow ternary's in React components, but nowhere else?

More of a discussion overall, since the code here is OK based on our docs.
https://docs.reactioncommerce.com/reaction-docs/master/styleguide#methods

if (isActionViewOpen === false) {
Session.set("productGrid/selectedProducts", []);
}
const isAdmin = Reaction.hasPermission("createProduct", Meteor.userId());

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Mar 9, 2018

Member

Can we do this outside of a composer, and then get rid of the composer since it's the only thing inside of it? Or does the admin check need to be run inside a composer?

This comment has been minimized.

Copy link
@mikemurray

mikemurray Mar 9, 2018

Member

It's a reactive data source, so if your permissions change, the grid can reflect that change with the admin/customer product grid.

return false;
}

const isInitialLoad = this.state.initialLoad === true;

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Mar 9, 2018

Member

Can this just be const isInitialLoad = this.state.initialLoad;?

This comment has been minimized.

Copy link
@mikemurray

mikemurray Mar 9, 2018

Member

The productsContainerAdmin.js was moved with git mv from productsContainer.js so it's a verbatim copy. Perhaps the original author intended to be triply sure that initialLoad was true/false and not just undefined or a string.

return {
...applyProductRevision(product)
// additionalMedia,
// primaryMedia

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Mar 9, 2018

Member

Can we remove these commented values?

This comment has been minimized.

Copy link
@mikemurray

mikemurray Mar 9, 2018

Member

will do

*/
export const Catalog = new Mongo.Collection("Catalog");

// Catalog.attachSchema(Schemas.Catalog);

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Mar 9, 2018

Member

Can we remove this commented code?

This comment has been minimized.

Copy link
@mikemurray

mikemurray Mar 9, 2018

Member

will remove, and add a TODO to attach a schema.

* @param {object} sort - sorting to be applied to the product find
* @return {MongoCursor} Mongo cursor object of found products
*/
Meteor.publish("Products/grid", function (productScrollLimit = 24, productFilters, sort = {}) {

This comment has been minimized.

Copy link
@kieckhafer

kieckhafer Mar 9, 2018

Member

Why do we use a capital P in this method as opposed to lowercase?

This comment has been minimized.

Copy link
@mikemurray

mikemurray Mar 9, 2018

Member

I think it was to match the name of the Products publication and collection name.

@kieckhafer

This comment has been minimized.

Copy link
Member

kieckhafer commented Mar 11, 2018

@willopez and I did a pair review on this.

There are a few issues related to the Sold Out, Backorder and Low Inventory badges, which is the 2nd test step, but we were seeing the same issues on other branches so I wouldn't say it's a blocker for this PR.

I will make a new ticket for those issues.

Aside from that, this looks like a good PR.

@willopez

This comment has been minimized.

Copy link
Member

willopez commented Mar 12, 2018

I Agree with @kieckhafer, the issues we are seeing are related to badges are existing issues. :shipit:

};

beforeEach(function (done) {
Collections.Products.direct.remove({});

This comment has been minimized.

Copy link
@willopez

willopez Mar 12, 2018

Member

Note: The direct will no longer be needed after the collections hooks are removed, and this collection hook will need to be removed.

fix: updated filterProducts function to look for the workflow.status …
…active for the shop and not the products, this fixed our failing products test
@nnnnat

This comment has been minimized.

Copy link
Member Author

nnnnat commented Mar 12, 2018

@spencern, @mikemurray and I were able to resolve the failing test, I believe everything else has been approved and we're ready to merge.

I've changed the name of the PR a few times and can't seem to get the WIP check to update.

@nnnnat nnnnat changed the title (refactor): 3662 customer product grid publishing WIP (refactor): 3662 customer product grid publishing Mar 12, 2018

@nnnnat nnnnat changed the title WIP (refactor): 3662 customer product grid publishing (refactor): 3662 customer product grid publishing Mar 12, 2018

@mikemurray mikemurray changed the base branch from feat-1221-aldeed-npm-cfs to release-1.9.0 Mar 13, 2018

@mikemurray
Copy link
Member

mikemurray left a comment

Merged changes from release-1.9.0 and fixed an issue with product grid price range when undefined.

Re-tested and it looks good to merge so we can start merging other dependent PRs into release-1.9.0.

@mikemurray mikemurray merged commit 499049f into release-1.9.0 Mar 13, 2018

1 of 3 checks passed

WIP work in progress – do not merge!
Details
ci/circleci CircleCI is running your tests
Details
security/snyk No dependency changes
Details

@mikemurray mikemurray deleted the refactor-3662-nnnnat-product-publishing branch Mar 13, 2018

@mikemurray mikemurray referenced this pull request Mar 14, 2018

Merged

Release 1.9.0 #3941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.