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

feat: Add integration to shiprocket, delhivery, aramex, envia, shippo #46

Closed

Conversation

ravibharathi656
Copy link

As a part of this PR, I am introducing a new doctype called Shipping Provider to refactor the existing design and introducing the following integration.

  • Shiprocket (In Indian context)
  • Delhivery (In Indian context)
  • Aramex (In Oman context)
  • Envia (In Indian context)
  • Shippo (In US context)

The existing ones such as letmeship, sendcloud is not affected by this refactoring. Once this is in, will do it as a separate PR.

Copy link
Collaborator

@barredterra barredterra left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looking forward to integrate these providers.

Regarding the Shipping Provider doctype: when you want to contribute new features, it's a bad idea to change the architecture at the same time. Please stick to the existing architecture. I'm sure there are pros and cons to both. But I think there is not a good reason to change or use two different architectures at the same time.

@Vigneshsekar
Copy link
Collaborator

@barredterra ,

As the existing architecture doesn't support having different shipment accounts for different companies within the same site, can we consider migrating LetMeShip and Send Cloud to the new doctype way Shipping Provider?

Also adding 5 more single doctypes for 5 integrations this PR brings may not be good too. What do you suggest?

@Vigneshsekar
Copy link
Collaborator

@ravibharathi656 , the currency issue is resolved in another PR. You can remove it, rebase and push it again.

@ravibharathi656
Copy link
Author

@barredterra , @Vigneshsekar ,

I used the sync fork feature of GitHub and it closed this PR with a force pushed commits. Let me open a new PR. I will also remove my changes related to the currency as that is taken care in #48 .

Sorry for this inconvenience.

@ravibharathi656
Copy link
Author

@barredterra I modified and raised the new PR #50

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.

3 participants