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

feat: Create product hash of published product properties #4336

Merged
merged 81 commits into from
Jun 22, 2018

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Jun 18, 2018

Resolves #4264
Impact: minor
Type: feature

Issue

After the removal of revisions, we need a way to allow an admin user to see if a product has been updated. Saving a hash of the data in it's current state, to allow for comparison, will allow the admin user to see if changes have been made.

Solution

Use the object-hash npm library to create a hash based on specific fields of a product that we want to track. Save this hash to the product itself, to check against to see if fields have been updated (ticket #4281)

Most fields are included in this hash. The primary excluded fields are system generated auto-fields such as createdAt and updatedAt, and the hash itself.

Additional notes:

  • Moved the function that created the product data to be saved to the Catalog into it's own file, createCatalogProduct, so we can keep it separate incase we ever want to create / see the catalog data without publishing it at the same time.
  • Added a migration to add the hash to products which have already been published. Although migration 24 already will add the hash, it's already been released to master so this is needed for anyone past 24.

Breaking changes

The function createCatalogProduct has been moved into it's own file. This should not create any issues, as it was not an exported function, but just be aware.

Testing

  1. Open Robo3T to see the Products and Catalog collections
  2. Create / edit a product
  3. See the hash update inside both collections

OR

Start the app and see the hash on a product that's already created, via migration.

@kieckhafer kieckhafer changed the base branch from master to release-1.13.0 June 18, 2018 22:13
@kieckhafer kieckhafer changed the title [WIP] feat: Create product hash of published product properties feat: Create product hash of published product properties Jun 19, 2018
@kieckhafer kieckhafer requested a review from nnnnat June 19, 2018 16:58
@kieckhafer kieckhafer changed the title feat: Create product hash of published product properties [WIP] feat: Create product hash of published product properties Jun 19, 2018
aldeed
aldeed previously requested changes Jun 21, 2018
"hash": {
type: String,
optional: true,
defaultValue: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need any default value. Is there a reason to have it?

Also, can we call it publishedProductHash for clarity?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't make sense, but it was defaulting to {} before I added this, which was causing an error. As soon as I added it, the error went away.

I changed a few things after that so I'll try to remove it and update the name.

workflow: {
status: product.workflow.status
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

  • isBackorder, isLowQuantity and isSoldOut don't exist on products, so remove those
  • Remove positions because we don't support it anymore
  • The media prop that we generate on the CatalogProduct will need to be part of the hash (should show as changed if any media have changed or the order of the media)
  • I don't think workflow is needed because we don't publish it

Also, I believe you will need to add all of the descendent variants and options to the hash. I guess this is why I was thinking the catalog schema would be hashed rather than product schema. Even though we are storing the hash on Product, we need to tell them to republish if anything beneath that product has changed, which includes all properties of all its variants and options.

}
},
{ upsert: true }
);
Copy link
Contributor

Choose a reason for hiding this comment

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

upsert: true and $setOnInsert are not needed because we know for sure it already exists, because we have productId.

* @param {Object} collections - Raw mongo collections
* @return {Object} updated product if successful, original product if unsuccessful
*/
export default async function hashProduct(productId, collections) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is updating DB, it belongs in no-meteor/mutations rather than no-meteor/utils

barcode: product.barcode,
createdAt: product.createdAt,
description: product.description,
hash: product.hash,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any reason why hash would be needed on the CatalogProduct

@kieckhafer kieckhafer dismissed aldeed’s stale review June 21, 2018 18:37

@aldeed addressed your comments, ready for another look.

@nnnnat nnnnat self-assigned this Jun 22, 2018
Copy link
Contributor

@nnnnat nnnnat left a comment

Choose a reason for hiding this comment

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

LG2M!

@nnnnat nnnnat changed the base branch from release-1.13.0 to release-1.14.0 June 22, 2018 17:58
@nnnnat
Copy link
Contributor

nnnnat commented Jun 22, 2018

@kieckhafer I changed this PR to merge into release 1.14.0 since 1.13.0 is out now. We've got a few conflicts if you can resolve I'll merge.

@nnnnat nnnnat merged commit 73592c4 into release-1.14.0 Jun 22, 2018
@nnnnat nnnnat deleted the feat-4264-kieckhafer-createProductHash branch June 22, 2018 18:47
@spencern spencern mentioned this pull request Jun 25, 2018
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.

5 participants