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

Replace CFS #3782

Merged
merged 25 commits into from Mar 9, 2018

Conversation

Projects
None yet
3 participants
@aldeed
Copy link
Member

commented Feb 16, 2018

Resolves #1221
Impact: breaking
Type: feature

Issue

The Meteor CollectionFS packages are not maintained. Although they work fine in most cases, we have limited ability to troubleshoot and fix bugs, and we can't easily implement additional stores and file types.

Solution

The Meteor CollectionFS packages are gone and are replaced with new, similar but cleaner and more modern NPM packages. These live here: https://github.com/reactioncommerce/reaction-file-collections

  • The main client and server files for file handling are now in /imports/plugins/core/files
  • There is now a MediaRecords backing collection exported with the other collections, and a Media MeteorFileCollection exported from the files plugin. MediaRecords is the equivalent of what you previously got from doing Media.files, i.e., the direct collection reference. But going forward it should just be imported like any other collection.
  • I've consolidated some of the duplicated code around getting product images (see getPrimaryMediaForItem). Also Reaction.Email.getShopLogo for emails. There may even be places where more reuse could happen, but sometimes the Media selector is slightly different and we'd have to think about whether those differences are truly necessary.
  • I created a quick React component to replace the brand image uploader/selector in Shop Settings. This probably needs some design love, but it's functional. We may want to tackle better design for it as a separate ticket.
  • I did NOT remove MediaRecords collection hooks. Will create a separate ticket for that.
  • Added indexes on MediaRecords collection to make those queries faster.

Breaking changes

If you've saved the file URLs anywhere, they're now different.

/assets/files/:collectionName/:fileId/:filename

becomes

/assets/files/:collectionName/:fileId/:primaryStoreName/:filename

and

/assets/files/:collectionName/:fileId/:filename?store=storeName

becomes

/assets/files/:collectionName/:fileId/:storeName/:filename

I've deleted some unused Blaze templates rather than update URL handling within them:

  • shopBrandImageOption
  • ordersListItems
  • select
  • upload
  • productMetaField
  • productMetaFieldForm
  • metaComponent
  • productDetailEdit
  • productDetailField
  • productImageGallery
  • imageDetail
  • imageUploader
  • productSocial
  • variantList
  • variant

Media-related publishing is changed and improved:

  • "CartItemImage" publication is removed
  • "CartImages" now takes an ID
  • Added "ProductGridMedia" to replace Media being included with the products publication for the grid
  • Added "ProductMedia"
  • Added "OrderImages", similar to CartImages, used for order now rather than reusing CartImages

Testing

Test each of these

  • logged in as admin
  • logged in as non-admin
  • not logged in
  1. Grid - Product images show up and look correct for each different view option
  2. PDP - Product images look correct
  3. Emails
    • make sure the correct shop logo is used in all emails, or the correct fallback logo if there isn't one

And test these only as admin:

  1. Grid - thumbnail image in the panel appears
  2. Shop settings
    • Click to add brand images
    • Drop brand images
    • Click brand image to change
    • Delete brand images (and if you cancel, make sure not deleted)
    • Try to upload non-images (shouldn't be allowed)
  3. PDP
    • Click + to add images
    • Drop images
    • Try to upload non-images (shouldn't be allowed)
    • Drag to reorder images
    • Zoom view should work when not an admin
    • Delete product images (and if you cancel, make sure not deleted)
  4. Orders
    • Make sure correct photo appears next to each product in invoice
  5. Cloning products with media
    • make sure it succeeds and all images are also copied, in all stores
  6. Shopify Import
    • Make sure product images come over
  7. Anywhere else that you see was touched when reviewing the code diff

@aldeed aldeed force-pushed the feat-1221-aldeed-npm-cfs branch 5 times, most recently Feb 19, 2018

@aldeed

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2018

@spencern snyk is complaining about the google-cloud packages used by tus-node-server, but we don't even use those adapters so we're not affected. However, I don't have a button to ignore that one. Are you able to ignore it?

feat(media): Replace CFS
BREAKING CHANGES

@aldeed aldeed force-pushed the feat-1221-aldeed-npm-cfs branch to 4be65a9 Feb 23, 2018

@aldeed aldeed changed the title [WIP] feat: Replace CFS Replace CFS Feb 23, 2018

@aldeed aldeed changed the base branch from release-1.8.0 to master Feb 23, 2018

@aldeed

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2018

@spencern Description has been updated.

I did notice that there are sometimes max recursion errors in the console when clicking around on product grid. I think it's because I'm setting primaryMedia and additionalMedia as properties on each product in imports/plugins/included/product-variant/containers/productsContainer.js. At some point something in Meteor must be trying to parse/clone the product document and choking on the FileRecord instances because they aren't cloneable. The solution is probably to pass through those media items as a separate prop to the React components (there are a few layers), and then let the final component that uses them (the renderMedia and renderAdditionalMedia methods of ProductGridItems) grab them off some other prop object instead.

@spencern
Copy link
Member

left a comment

Several issues that we need to resolve before we can merge this.

  1. After loading products and images, clicking on the grid will produce a "Maximum call stack size exceeded" error. @aldeed notes this error in a comment to this PR.

  2. Tests. Currently 3 of the product tests are timing out. We'll need to isolate these, determine why they are failing and resolve the issues with them.

  3. Snyk issues. These are also noted by @aldeed in. a comment. Currently only dev dependencies so we can probably safely ignore for now, but we should see if there's an upgrade path that will resolve these before muting.

@spencern spencern self-assigned this Feb 26, 2018

@spencern

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

Assigning myself to try to get this across the finish line

@spencern

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

In regards to Blocker 1
The issue is definitely related to the way images are passed through the props as @aldeed suggests. By commenting out these two lines https://github.com/reactioncommerce/reaction/blob/feat-1221-aldeed-npm-cfs/imports/plugins/included/product-variant/containers/productsContainer.js#L211-L212
We lose images on the grid, but the grid clicking starts working again and we no longer see the Maximum call stack size exceeded issue.

I'm going to experiment with passing the product images through a different prop.

(fix): Pass media in separate props to productGridItems
This resolves the issue where clicking around the product grid would cause a `Maximum call stack size exceeded`
@spencern

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

I've resolved the issue with the product grid exceeding the max call stack size by passing product media to the grid item via a separate prop from the product.

@spencern
Copy link
Member

left a comment

Currently two outstanding issues:

  • Cloning doesn't work. Haven't been able to track down where this would be coming from
  • Drag and drop arrangment of images on the PDP doesn't work

As all users

  • Grid - Product images show up and look correct for each different view option
  • PDP - Product images look correct
  • Emails (fixed pending PR to file-collections lib)
  • make sure the correct shop logo is used in all emails, or the correct fallback logo if there isn't one (fixed pending PR to file-collections lib)

Admin Only

  • Grid - thumbnail image in the panel appears (fixed)
  • Cloning products with media (error cloning) (fixed in reactioncommerce/reaction-file-collections#3)
  • make sure it succeeds and all images are also copied, in all stores (error cloning)

Shop settings

  • Click to add brand images
  • Drop brand images
  • Click brand image to change
  • Delete brand images
  • Delete brand images and cancel, make sure not deleted
  • Try to upload non-images (shouldn't be allowed)

PDP

  • Click + to add images
  • Drop images
  • Try to upload non-images (shouldn't be allowed) (no error or warning, but not permitted)
  • Drag to reorder images
  • Zoom view should work when not an admin
  • Delete product images (and if you cancel, make sure not deleted)

Orders

  • Make sure correct photo appears next to each product in invoice

Shopify Import

  • Make sure product images come over
@mikemurray
Copy link
Member

left a comment

I receive the following error when trying to use the search.

TypeError: Cannot read property 'BCTMZ6HTxFSppJESk' of undefined
    at app.js:54644
    at Array.map (<anonymous>)
    at ProductGrid._this.renderProductGridItems (app.js:54641)
    at ProductGrid.render (app.js:54673)
    at modules.js:152451
    at measureLifeCyclePerf (modules.js:151731)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (modules.js:152450)
    at ReactCompositeComponentWrapper._renderValidatedComponent (modules.js:152477)
    at ReactCompositeComponentWrapper.performInitialMount (modules.js:152017)
    at ReactCompositeComponentWrapper.mountComponent (modules.js:151913)

localhost_4000

@aldeed aldeed changed the base branch from master to release-1.9.0 Mar 8, 2018

@aldeed

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2018

@spencern Reordering the media gallery now works when revisions are disabled. I think something weird is still happening with revisions logic, which cancels out the reordering sometime. Maybe a publication issue. I might just go ahead and move the MediaRecords revisions out of collection-hooks to be able to understand better what is happening, since we need to do that anyway. But feel free to take another look and let me know if the problem with revisions is obvious to you.

@spencern

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

@aldeed I'll take a look

@aldeed

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2018

@mikemurray The search error should be fixed now, and images appearing in search. @spencern FYI To get images showing in search, I had to move the productMediaById composer logic from productsContainer to productGridContainer because search uses productGridContainer directly.

@spencern

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

@mikemurray does this change impact the work you and @nnnnat are doing with the product grid?

@spencern

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

@aldeed In regards to the issue with rearranging images not working, there seems to be an issue with the MediaRecords.update call here: https://github.com/reactioncommerce/reaction/blob/feat-1221-aldeed-npm-cfs/imports/plugins/core/revisions/server/methods.js#L25

It's returning false and failing to update the MediaRecords, even if the media record is available and has the correct ID, and the Revisions.update that happens a few lines down succeeds.

I think the false return must be coming from the collection hooks. Digging into that now.

@spencern

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

And I've got a hack fix that's working now.

The issue is definitely stemming from https://github.com/reactioncommerce/reaction/blob/feat-1221-aldeed-npm-cfs/imports/plugins/core/revisions/server/hooks.js#L268

The way it's written right now, there's no way it could be anything but false, which then causes the original update not to go through.

@aldeed we can discuss my approach and see if there's any other spots we'll need to update.

@mikemurray

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

@spencern, @nnnnat and I removed all logic from the productsContainer and split it into productsContainerAdmin and productsContainerCustomer. productsContainer is now just used to determine if you're an admin or customer and serves the correct product grid.

@spencern
Copy link
Member

left a comment

Worked with @aldeed to get this ready and we've fixed all of my blockers. 👍

@spencern spencern merged commit a68f4ab into release-1.9.0 Mar 9, 2018

3 checks passed

WIP ready for review
Details
ci/circleci Your tests passed on CircleCI!
Details
security/snyk No new issues
Details

@spencern spencern referenced this pull request Mar 14, 2018

Merged

Release 1.9.0 #3941

@spencern spencern deleted the feat-1221-aldeed-npm-cfs branch Mar 15, 2018

@spencern spencern referenced this pull request Mar 22, 2018

Closed

Incorrect featured image displayed after zooming #4057

3 of 3 tasks complete
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.