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

Merge development into marketplace #2529

Merged
merged 116 commits into from
Jul 11, 2017
Merged

Conversation

spencern
Copy link
Contributor

@spencern spencern commented Jul 6, 2017

Merges latest development into marketplace
Resolves all conflicts (a lot!)

@spencern spencern changed the title Spencer merge dev marketplace [WIP] Spencer merge dev marketplace Jul 7, 2017
I _swear_ this worked before, but I’m not sure what got changed and when I go back to older commits, the same issue is present.
The apparent problem is that the Security.permit / Security.deny collection permission security methods are running before a users Id has been loaded.

The fix here is to create a new security method `ifHasRoleForActiveShop` that moves checking `Reaction.getShopId()` to the security method instead of on the `permit` or `deny` method.
@spencern
Copy link
Contributor Author

spencern commented Jul 7, 2017

I swear this worked before, but I’m not sure what got changed and when I go back to older commits, the same issue is present.
The apparent problem is that the Security.permit / Security.deny collection permission security methods are running before a users Id has been loaded.

The fix here is to create a new security method ifHasRoleForActiveShop that moves checking Reaction.getShopId() to the security method instead of on the permit or deny method.

@zenweasel would you check this out when you get a chance. I think it's ready to merge to marketplace now and would you review the changes in /server/security/collections.js specifically.

After this merges I think we should be able to get the team to start working off of the marketplace branch instead of development for most tasks.

jshimko and others added 2 commits July 8, 2017 15:30
@brent-hoover
Copy link
Collaborator

@spencern What exactly did you want me to review? Ddi you want me to test it?

@spencern
Copy link
Contributor Author

@zenweasel Yeah, I should have put "to test" steps up.

I was specifically hoping you could look at the new security method I added to make the active shop admin permissions work correctly for client side database ops.

https://github.com/reactioncommerce/reaction/blob/spencer-merge-dev-marketplace/server/security/collections.js#L43-L53

I'm pretty sure it's fine, but seems like the type of code that should be reviewed.

@spencern
Copy link
Contributor Author

Additionally,

To test:

  1. Start server
  2. Create a new account
  3. Click "Become a Seller" in the users profile
  4. Change the name or other shop details of the new shop that gets created.

@spencern spencern changed the title [WIP] Spencer merge dev marketplace Spencer merge dev marketplace Jul 10, 2017
@spencern spencern changed the title Spencer merge dev marketplace Merge development into marketplace Jul 10, 2017
import PropTypes from "prop-types";
import { Icon } from "/imports/plugins/core/ui/client/components";

class EmailTableColumn extends Component {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably in development but why is this called EmailTableColumn?

@brent-hoover
Copy link
Collaborator

If I create an account and then click "Become a seller" I get a bunch of console errors:

TypeError: Cannot read property 'brandAssets' of undefined
    at Brand.getLogo (brand.js:21)
    at Brand.render (brand.js:30)
    at modules.js?hash=1e7eb5b…:107993
    at measureLifeCyclePerf (modules.js?hash=1e7eb5b…:107272)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (modules.js?hash=1e7eb5b…:107992)
    at ReactCompositeComponentWrapper._renderValidatedComponent (modules.js?hash=1e7eb5b…:108019)
    at ReactCompositeComponentWrapper._updateRenderedComponent (modules.js?hash=1e7eb5b…:107943)
    at ReactCompositeComponentWrapper._performComponentUpdate (modules.js?hash=1e7eb5b…:107921)
    at ReactCompositeComponentWrapper.updateComponent (modules.js?hash=1e7eb5b…:107842)
    at ReactCompositeComponentWrapper.receiveComponent (modules.js?hash=1e7eb5b…:107744)

It repeats with can't find locales, language of undefined

@spencern
Copy link
Contributor Author

@zenweasel I'd seen those errors before but couldn't reproduce them. I'll see if I can address those now

@brent-hoover
Copy link
Collaborator

Testing this again with a full reset I am not seeing it and can't reproduce it either. I had done a full reset before as well so I don't know what the deal it.

package.json Outdated
"^meteor/aldeed:simple-schema": "<rootDir>/imports/test-utils/__mocks__/meteor/aldeed-simple-schema",
"^meteor/tmeasday:publish-counts": "<rootDir>/imports/test-utils/__mocks__/meteor/aldeed-simple-schema",
Copy link
Collaborator

@brent-hoover brent-hoover Jul 11, 2017

Choose a reason for hiding this comment

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

I think this is wrong. Should be tmeasday:publish-counts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this is wrong in development too

Copy link
Contributor Author

@spencern spencern Jul 11, 2017

Choose a reason for hiding this comment

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

@@ -85,7 +89,7 @@ export default {
},
/**
* registerTemplate
* registers Templates into the Templates Collection
* registers Templates into the Tempaltes Collection
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

const packages = this.Packages;
// for each shop, we're loading packages in a unique registry
// Object.keys(pkgConfigs).forEach((pkgName) => {
for (const packageName in packages) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if packages is an array can you just use for..of to iterate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably could, seems like that would be better. Could lose the guard then I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, packages is an object where the keys are the packageName, not an array.

};

// Authorized content curators fo the shop get special publication of the product
// all all relevant revisions all is one package
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in comment

@@ -271,7 +216,7 @@ Meteor.publish("Product", function (productId) {
];
}

// Everyone else gets the standard, visible products and variants
// Everyone else gets the standard, visibile products and variants
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed from correctly spelled to incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah, wonder how that snuck in.

@brent-hoover
Copy link
Collaborator

I expect that everything I noted is probably also on development so this looks good to me. I checked the code you noted @spencern and it seems fine to me as far as my understanding of what it's supposed to do.

@spencern spencern merged commit 1d35b29 into marketplace Jul 11, 2017
@spencern spencern deleted the spencer-merge-dev-marketplace branch July 11, 2017 05:01
@spencern spencern mentioned this pull request Oct 11, 2017
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.