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

Performance/Refactor remove revision control #4238

Merged
merged 58 commits into from
May 22, 2018

Conversation

mikemurray
Copy link
Member

@mikemurray mikemurray commented May 10, 2018

Resolves #4237
Impact: breaking
Type: performance/refactor

Issue

Remove the Revision control system in favor of the Catalog as having both is redundant.

Solution

Remove all code, hooks, collections, and packages related to revision control.

Breaking changes

  • The revision control system is removed, any custom packages that depend on it will no longer work.
  • removed applyProductRevision from lib/api/products.js
  • Cannot undo changes when editing a product
  • PDP page will have an instant reflection of changes. Product grid will only see changes upon publishing.

Testing

  1. As an admin
  2. Add products, see that the changes show up immediately on PDP for customer and admin
  3. Check the product grid has no changes for the customer, but immediate change for admin
  4. Publish product, see product grid change for customers
  5. Add products to cart and checkout
  6. Fulfill orders and verify that inventory updates on the product via the PDP admin tools

@mikemurray mikemurray added this to the Democrat milestone May 14, 2018
@spencern spencern modified the milestones: Democrat, Elbert May 15, 2018
@mikemurray mikemurray changed the title [WIP] Performance/Refactor remove revision control Performance/Refactor remove revision control May 18, 2018
@mikemurray mikemurray requested a review from aldeed May 18, 2018 22:28
@spencern
Copy link
Contributor

@aldeed I think we should probably merge this into Release 2.0 and not into 1.12.0

aldeed
aldeed previously requested changes May 21, 2018
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

@mikemurray Nice work. I found a few things that seem like they should be removed.

  • In /imports/plugins/core/catalog/client/containers/publishContainer.js, calls Meteor.call("revisions/discard"
  • /imports/plugins/core/catalog/client/components/publishControls.js still has renderUndoButton method but it isn't called.
  • In /imports/plugins/core/ui/client/components/media/mediaItem.js, there are some revision checks that should not be needed now
  • In the json files in /private/data/i18n, there is a revisions object. It seems like most of these are not needed anymore. Maybe keep only unpublishedChanges and publishChanges?
  • Should be safe to remove the Revisions line from /imports/collections/defineCollections.js

Still testing a few more things, but I'll start with those for my comments.

@aldeed aldeed changed the base branch from release-1.12.0 to release-2.0.0 May 21, 2018 21:45
@mikemurray mikemurray dismissed aldeed’s stale review May 21, 2018 22:22

Fixed outlines issues

@mikemurray
Copy link
Member Author

@aldeed Fixed your current set of issues. Let me know if you find any more.

@aldeed
Copy link
Contributor

aldeed commented May 22, 2018

@mikemurray This is looking good to me. The only thing I noticed was that all the indicators for unpublished changes are gone. Is there no way to keep those? For example, by diffing Products w/ Catalog? I know the schemas are different so it's not necessarily easy.

@mikemurray
Copy link
Member Author

@aldeed We would need to diff or otherwise know that there have been changes since the last publish.

@aldeed aldeed merged commit accc6b7 into release-2.0.0 May 22, 2018
@spencern
Copy link
Contributor

Issue that was created to flag a product which has unpublished changes: #4264

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.

3 participants