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

Issue with the ReactionImport class #646

Closed
boboci9 opened this Issue Jan 5, 2016 · 15 comments

Comments

6 participants
@boboci9
Contributor

boboci9 commented Jan 5, 2016

Hi,

I am testing the new version with multiple shops added, this worked correctly in the previous version but now the packages are multiplied but no to the correct shop. The main ReactionCore.getShopId() will be added to every registered entry in the Packages collection instead of each shop's id therefore one package will be registered several times with the same id causing multiple items in the user dropdown and other issues like this.
Let me know if I can help with a PR, I am debugging the issue too and trying to find a solution.
So far I know that the ReactionRegistry.loadPackages function runs correctly through every shop and the ReactionImport.package({ name: pkgName, icon: config.icon, enabled: !!config.autoEnable, settings: config.settings, registry: config.registry, layout: config.layout }, shopId); function is called with the correct shopId so not sure yet where thing go wrong.

@aaronjudd

This comment has been minimized.

Contributor

aaronjudd commented Jan 5, 2016

a PR would be appreciated. It'd also be great if you could give us an overview on how you implemented the multiple shops for #357. I'd like to get some tests going so we don't keep breaking your implementation, and also see how closely your strategy aligns to what we were planning. There's quite a few people interested ;-)

@tdecaluwe

This comment has been minimized.

Contributor

tdecaluwe commented Jan 5, 2016

@boboci9 Do you have some json import files to test with? Or a walkthrough to reproduce the problem?

@boboci9

This comment has been minimized.

Contributor

boboci9 commented Jan 6, 2016

Hi, All you need to do is add a new entry in the Shops collection, in this case you should have in the DB packages registered with the new shopId but all the registry entries are for the same shopId. Let me know if you need any other info.

@boboci9

This comment has been minimized.

Contributor

boboci9 commented Jan 6, 2016

@aaronjudd I will go over the changes you added to #357, we have previously discussed some details regarding what you have been planning and I kept our implementations to those.

  • Regarding the orders I have implemented the solution I described in #154, we didn't PR that as you had your code back then in Apr in coffee and we were working with JS. In our tests this has been working well, every shop admin has access only to his own orders (suborders) while the main marketplace admin has access to the parent orders as well.
  • The shipping options are set by every shop admin and when different shop items appear in the same order the user must select a shipping option for every shop in order to continue to the order payment.
  • Regarding the products I publish all the products from the shops which have multiVendorEnabled flag enabled, this is set by the main admin when their registration is approved.
@boboci9

This comment has been minimized.

Contributor

boboci9 commented Jan 6, 2016

Finally found the issue, it is not coming from the ReactionImport class, it is coming from the reaction-schemas package, Packages schema. The autovalue is set to the main shopId so no mater what shopId you send it will be ignored.

 "shopId": {
    type: String,
    index: 1,
    autoValue: ReactionCore.shopIdAutoValue,
    label: "PackageConfig ShopId",
    optional: true
  },

I commented out the autovalue and the packages are registered correctly but I didn't send a PR with this as I am not sure what you would like to have instead.

@boboci9

This comment has been minimized.

Contributor

boboci9 commented Jan 6, 2016

Same issue happens when a seller tries to add a new product, the product is inserted to the right shop and then automatically moved to the main shopId when you try to edit it. The products collection has the autoValue set the same way and when I comment it out I can edit the product without switching it to the main shop.

@stabenfeldt

This comment has been minimized.

stabenfeldt commented Jan 8, 2016

I am testing the new version with multiple shops added

Is the multi tendant marketplace feature completed? I thought I saw it listed on the roadmap page. Great news if it's done! :-) Is it production ready?

@aaronjudd

This comment has been minimized.

Contributor

aaronjudd commented Jan 8, 2016

@boboci9

  shopIdAutoValue: function () {
    // we should always have a shopId
    if (ReactionCore.getShopId()) {
      if (this.isSet && Meteor.isServer) {
        return this.value;
      } else if (Meteor.isServer || Meteor.isClient && this.isInsert) {
        return ReactionCore.getShopId();
      }
      return this.unset();
    }
  },

I think this is (by design), preventing shop inserts from the client. However, it looks like it should allow you to set / provide a shopId and insert server side. (or with the shop/createShop method). I need to dig in a little more I guess and see what if that's not working on the server side.

to test, maybe:

if (this.isSet) {
 return this.value;

But I'm not sure about the ramifications.

@aaronjudd

This comment has been minimized.

Contributor

aaronjudd commented Jan 8, 2016

@stabenfeldt it's still a work in progress. Mostly the work remaining is UI / management related. You could make modifications right now to use as a marketplace (single tenant, multiple shops), and we keep moving towards providing that feature in the future. We do try to keep the schema and collections designed for these use cases though

@boboci9

This comment has been minimized.

Contributor

boboci9 commented Jan 8, 2016

@aaronjudd I will add your suggestion in my version and start testing it out so I can help you out with any unexpected ramification.
Regarding my overview on the multiple shops I am discussing them on gitter so we can have a real conversation about them.

@boboci9

This comment has been minimized.

Contributor

boboci9 commented Jan 8, 2016

@aaronjudd: The issue with this will be for example for the Products collection you are calling the products/updateProductField from the client. When this is done by a certain seller in his own shop the shopId will jump back to the main shopId instead of staying holding the seller's shopId as created through the insert.
For the import packages it should work as that is called from the server registry.js file

@boboci9

This comment has been minimized.

Contributor

boboci9 commented Jan 8, 2016

@aaronjudd Also when an order is viewed by a seller of a separate shop the order's shopId will jump to the mainShopId instead of keeping the value it has been created with because the move to next workflow step is called from the client side.
Adding these as I find them during testing different use cases so you have more prototypes for testing.

@aaronjudd aaronjudd self-assigned this Feb 6, 2016

@aaronjudd aaronjudd added in progress and removed bug help wanted labels Feb 6, 2016

@aaronjudd aaronjudd added this to the Marketplace milestone Feb 6, 2016

@aaronjudd aaronjudd removed their assignment Mar 22, 2016

@aaronjudd aaronjudd added backlog and removed in progress labels Mar 22, 2016

@woakes070048

This comment has been minimized.

woakes070048 commented Mar 30, 2016

@aaronjudd what is the state of this?

@aaronjudd aaronjudd modified the milestones: v0.14.0, Marketplace Mar 30, 2016

@aaronjudd

This comment has been minimized.

Contributor

aaronjudd commented Mar 30, 2016

@woakes070048 I'm not sure if this is an issue any more, needs testing.

@woakes070048

This comment has been minimized.

woakes070048 commented Mar 30, 2016

@aaronjudd i guess i will install it and do some testing. i was reading somewhere and it mentioned that it was private and called reaction-drive

@aaronjudd aaronjudd modified the milestones: Import Migration , v0.14.0 Jun 28, 2016

@aaronjudd aaronjudd modified the milestones: Import Migration , v0.20.0 Sep 13, 2016

@kieckhafer kieckhafer closed this Feb 15, 2017

@kieckhafer kieckhafer added wontfix and removed backlog labels Feb 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment