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

fix: carriers not showing on merchant #1253

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

YavorGrancharov
Copy link
Contributor

Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.


Fixed carrier not showing on Merchant app.
https://www.loom.com/share/c1bf8ce9786e45b6a3f67b37ba4cb4d1

@YavorGrancharov YavorGrancharov added the type: bug 🐛 Something isn't working label Jul 14, 2020
@YavorGrancharov YavorGrancharov requested a review from evereq July 14, 2020 19:12
@YavorGrancharov YavorGrancharov self-assigned this Jul 14, 2020
@evereq
Copy link
Member

evereq commented Jul 15, 2020

@YavorGrancharov hm, I think your solution is wrong... I.e. you will show only carriers who deliver some orders, but that page should display ALL Carriers assigned to merchant, not only those who deliver (or will deliver) something...

Could it be all code you need was actually just this part:

	this.carrierRouter
					.getAllActive()
					.subscribe((carriers: Carrier[]) => {
						this.carriers = carriers.filter((c: Carrier) =>
							usedCarriers.includes(c.id)
						);
						loadData(this.carriers);
						this.carriers.length === 0
							? (this.showNoDeliveryIcon = true)
							: (this.showNoDeliveryIcon = false);
					});

?

@YavorGrancharov
Copy link
Contributor Author

I thought that all carriers connected with the current store are the once to be rendered! Will show them all, thanks!

@evereq
Copy link
Member

evereq commented Jul 15, 2020

@YavorGrancharov hm, YES, only carriers who connected to the store are those we need to pay on that app page! I.e. why you would want to show all carriers from 1000s of different merchants? My point is that such carriers may NOT have any orders yet, instead, you should check in warehouse how we store connected carriers and display only them.

I.e. @YavorGrancharov whole platform is a very logical system. I see we have a difficulties to understand that. Merchant have app (Tablet) to manage her/his store (or Warehouse). Why would he want to see all carriers ever? He wants to see his own carriers (people who deliver goods/products/services), no matter if they worked before already (did some deliveries) or not yet. There can be a pool of "shared" carriers (e.g. 100) which are used by multiple merchants. In such case, I think some of such carriers could be also assigned to warehouse (so that 5 different warehouses use some same carriers) etc. and so on. All is really like very logical.

Question: why you removed this.warehouseCarriersRouter and decided to use something else? Was it the problem it did not worked, why???

@YavorGrancharov
Copy link
Contributor Author

I tried another approach which was to get orders made from the store and took by certain carriers and show those carriers except canceled orders as there carrier is null. With this.warehouseCarriersRouter carriers were always empty!

@evereq
Copy link
Member

evereq commented Jul 15, 2020

OK, so as I explained - you did it completely wrong. Instead, it was better to figure out why existed method always return "empty" results and possible fix that (most probably you did something wrong and not assign carriers for example, that's why it returns empty list always)

@YavorGrancharov
Copy link
Contributor Author

Ok, will do as you suggested!

@evereq
Copy link
Member

evereq commented Aug 1, 2020

@YavorGrancharov what is the situation with this PR? Do you need any help?

@evereq
Copy link
Member

evereq commented Aug 4, 2020

@YavorGrancharov ??? @valiopld please talk with @YavorGrancharov if need any help on this one

@YavorGrancharov
Copy link
Contributor Author

YavorGrancharov commented Aug 7, 2020

@evereq can you please review this PR!

So what I made for this functionality:

  1. When "Use specific carriers" on Admin -> Store -> Manage Store is set to false, on Merchant -> Carriers we see all active carrires.
  2. When "Use specific carriers" is set to true but non selected and no previously shared carriers were used before on Merchant we see nothing (I can improve this and again show all active carriers on Merchant).
  3. When "Use specific carriers" is set to true and some or all carriers are selected we see only those carriers on Merchant + previously used shared carriers by the same Merchant.
  4. When "Use specific carriers" is set to false when it was previously set to true and there were any selected carriers, we keep those carriers in databse in case in the future we decide to set "Use specific carriers" back to true. Also when "Use specific carriers" is set to false at some point, we can't see previouly selected carriers for that store on Merchant instead we see all active ones.
  5. When "Use specific carriers" is false but we decide to add carriers from "Add Carriers" popup on Merchant, then "Use specific carriers" is automaticaly set to true and the selected carrers are automaticaly checked in the drodown menu under "Use specific carriers" option on Admin.
  6. When "Use specific carriers" is false and the store can use all shared carrires if we create new order either from Admin or from Merchant and if carrier took that order either from Admin or Carrier the usedCarriersIds array is now populated and we can see those carriers on Merchant.
  7. When we add carriers either from Admin using "Use specific carriers" dropdown or from Merchant -> "Add Carriers" popup, we store those carriers in "carriersIds" array in database and not in "usedCarriersIds" array.
  8. We populate "usedCarriersIds" only when shared or specific carriers or both take work either from Carrier or from Admin.
  9. Previously when from Admin -> Store -> Manage Store we try to update Warehouse and "Use specific carriers" was set either to true or false the "usedCarriersIds" array in database was allways emptied and all the carriers that previously took work were removed from the array. Now it works as it should by not touchung that array so that all previously used shared carriers remains there on Warehouse update.

I also provided a video below for preview!

https://www.loom.com/share/722a53ff452444019af825ab4adf2b19

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Something isn't working
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants