search

reactioncommerce / reaction

Mailchimp Open Commerce is an API-first, headless commerce platform built using Node.js, React, GraphQL. Deployed via Docker and Kubernetes.
https://mailchimp.com/developer/open-commerce/
GNU General Public License v3.0
12.34k stars 2.17k forks source link

Link for "Your order was accepted" is broken #2810

Closed janus-reith closed 6 years ago

janus-reith commented 7 years ago

getShopPrefix() in https://github.com/reactioncommerce/reaction/blob/master/server/api/core/core.js#L201

always returns the shop name as slug.

Plugins like the notifcation view use this method to determine the route. For single shops this results in a not found page, as the notifaction route is registered as /notifcations, and the Link routes to /shopname/notifications.

I see two options here:

  1. Modify getShopPrefix() to return "" for single shops. In /lib/api/helpers.js we had a similar issue with the getShopPrefix method and Paypal: https://github.com/reactioncommerce/reaction/pull/2563/files#diff-7143087245674b35fd0f58f5c1e8a70cR41

  2. Change the way getShopPrefix is used. Maybe leave it untouched, but replace the parts where it is used with an additional method, that first checks shop count and invokes it if necessary. Could possibly prevent additional things from breaking.

Steps to Reproduce

  1. Configure shop to take order (configure shipping/payment)
  2. Place an order
  3. Return to home page
  4. Click on the "bell" icon in the top nav
  5. Click on the "Your Order was accepted" link
  6. Observe that you get a 404 error
janus-reith commented 7 years ago

I just noticed that getShopPrefix() in /client/modules/core/main.js has the same issue. Gladly, as far as I can see, both methods are only used in a few occasions.

aaronjudd commented 7 years ago

@janus-reith I believe this is fixed in marketplace.

brent-hoover commented 6 years ago

@janus-reith Can you confirm if this is fixed?

janus-reith commented 6 years ago

Actually I still have this issue on the current branch. but need to verify that it is not related to my customizations. When I click on a notification as customer on my single-mercahnt shop, like "your order is being processed", it leads me to domain.tld/slug/notifications, while it should be domain.tld/notifications

Manually navigating to domain.tld/notifications works. From there, I click a customer notification, it leads me back to domain.tld/slug/notifications, which returns the not found page again.

This just applies to the the customer notifications, while clicking those for the shop manager, like "You have a new order" work properly and open the orders dashboard. I tested buying with my admin account, so it had both notification types aggregated.

janus-reith commented 6 years ago

Looking at /server/api/core/core.js in the master branch, getShopPrefix() callls getShopName(), and none of them seem to check for shop count. But maybe there have been changes somewhere else, to just call getShopPrefix in case there is more than one shop - DIdn't have the time yet to check that.

As there haven't been further responses here: Does clicking on order update notifications work properly for you on single merchant shops?

brent-hoover commented 6 years ago

Testing it now the "You have a new order" notification works and takes me to the dashboard but the "Your order was accepted" gives me a 404. So this would appear to still be broken in master