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

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

Closed
janus-reith opened this Issue Sep 8, 2017 · 6 comments

Comments

5 participants
@janus-reith
Copy link

commented Sep 8, 2017

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

This comment has been minimized.

Copy link
Author

commented Sep 8, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

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

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

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

@janus-reith

This comment has been minimized.

Copy link
Author

commented Jan 5, 2018

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

This comment has been minimized.

Copy link
Author

commented Jan 5, 2018

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?

@zenweasel

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

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

@spencern spencern added this to the Bugfix Sprint 1 milestone Feb 9, 2018

@zenweasel zenweasel changed the title Reaction.getShopPrefix() broken / used wrong => Notifcatons broken Link for "Your order was accepted" is broken Feb 14, 2018

@Akarshit Akarshit assigned Akarshit and unassigned Akarshit Feb 15, 2018

@spencern spencern modified the milestones: Bugfix Sprint 1, Release 1.9 Feb 20, 2018

spencern added a commit that referenced this issue Mar 5, 2018

@spencern spencern closed this Mar 5, 2018

@spencern spencern referenced this issue Mar 8, 2018

Merged

Release 1.9.0 #3941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.