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

[17.0][ADD] delivery_sendcloud_oca #852

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

ByteMeAsap
Copy link

@ByteMeAsap ByteMeAsap commented Jul 9, 2024

delivery_sendcloud_oca module provides sendcloud shipping integration with Odoo

This module mostly implements what's described in https://docs.sendcloud.sc/api/v2/shipping/

Full documentation for developers is in https://docs.sendcloud.sc/.

This module works for the Community Edition as well as the Enterprise Edition.

Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

LGTM! This module was moved and migrated from https://github.com/onesteinbv/addons-sendcloud. We (Onestein) have given permission to transition this to the OCA

@rousseldenis
Copy link
Contributor

@ByteMeAsap Could you rename this to delivery_sendcloud_ocaas it exists in Odoo enterprise?

Thanks

@rousseldenis rousseldenis added this to the 17.0 milestone Sep 20, 2024
@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch from efd86b3 to f9f5490 Compare September 20, 2024 08:54
@ByteMeAsap ByteMeAsap changed the title [17.0][ADD] delivery_sendcloud [17.0][ADD] delivery_sendcloud_oca Sep 20, 2024
@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch 2 times, most recently from c909c4c to 7160fea Compare September 20, 2024 10:18
@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch 2 times, most recently from ab46bed to fb0ba77 Compare September 20, 2024 15:01
@ByteMeAsap
Copy link
Author

@ByteMeAsap Could you rename this to delivery_sendcloud_ocaas it exists in Odoo enterprise?

Thanks

@rousseldenis , Yes , I have renamed the module. Can you help me with how do I go about resolving conflicts as there have been changes in the 17.0 branch since I had raised the PR a while back?

@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch 2 times, most recently from 17824cb to 3b5536f Compare September 26, 2024 05:58
Copy link

@Jan-Onestein Jan-Onestein left a comment

Choose a reason for hiding this comment

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

Looks good to me

@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch 3 times, most recently from a6364c1 to 4f240b0 Compare November 5, 2024 08:31
@ByteMeAsap
Copy link
Author

@OCA/delivery-carrier-maintainers, @rousseldenis , can we have this merged?

@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch from 4f240b0 to 63376d2 Compare November 28, 2024 13:43
@Jan-Onestein
Copy link

Can we please have this merged ??
Can some of you guys push this ??
( @rousseldenis / @gurneyalex / @jgrandguillaume / @simahawk / @JordiBForgeFlow / @rafaelbn / @tafaRU / @jbaudoux / @hparfr )

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

Code review


@api.depends("sendcloud_service_point_input", "delivery_type")
def _compute_sendcloud_service_point_required(self):
for carrier in self:
Copy link
Contributor

@rousseldenis rousseldenis Dec 9, 2024

Choose a reason for hiding this comment

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

self.update({"sendcloud_service_point_required": False})

Copy link
Author

Choose a reason for hiding this comment

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

@rousseldenis , I have updated the function

@api.depends()
def _compute_sendcloud_country_ids(self):
for carrier in self:
countries = self.env["sendcloud.shipping.method.country"].search(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do a search before the loop and then filter in it to improve performances.

Copy link
Author

Choose a reason for hiding this comment

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

Done !!


@api.onchange("delivery_type")
def _onchange_sendcloud_delivery_type(self):
self._sendcloud_set_countries()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really to be done in an onchange ? This can be bad from a UX point of view.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated to set the countries on write

deleted_parcels.append(parcel_code)
elif (
res.get("status") == "failed"
and res.get("message") == "This shipment is already being cancelled."
Copy link
Contributor

Choose a reason for hiding this comment

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

Could not this be translated ?

Copy link
Author

Choose a reason for hiding this comment

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

No, this cannot be translated

elif res.get("error", {}).get("code") == 404:
deleted_parcels.append(parcel_code) # ignore "Not Found" error
elif res.get("error"):
raise ValidationError(_("Sendcloud: %s") % res["error"].get("message"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use translation function arguments instead.

Copy link
Author

Choose a reason for hiding this comment

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

Done

def _compute_can_generate_return(self):
res = super()._compute_can_generate_return()
for carrier in self.filtered(lambda c: c.delivery_type == "sendcloud"):
carrier.can_generate_return = True
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use an update() on recordset instead.

Copy link
Author

Choose a reason for hiding this comment

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

Done !!

):
record.country_ids = record._sendcloud_get_countries_from_cache()

def _sendcloud_get_countries_from_cache(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the method name deos not reflect the current behavior.

Copy link
Author

Choose a reason for hiding this comment

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

Changed the method name

if existing.sendcloud_code not in existing_shipping_methods_map:
existing_shipping_methods_map[existing.sendcloud_code] = self.env[
"delivery.carrier"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

self.env["delivery.carrier"].browse() is semantically more correct.

shipping_method_country.product_id = item.product_id
shipping_method_country.enable_price_custom = item.enable_price_custom
else:
self.env["sendcloud.shipping.method.country.custom"].create(
Copy link
Contributor

Choose a reason for hiding this comment

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

Build a list of create values in the loop, then call create() once.

Copy link
Author

Choose a reason for hiding this comment

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

Done !

@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch from 63376d2 to d809bbd Compare December 10, 2024 06:59
Updated requirements.txt file and changed qty_done to quantity_product_uom for stock move lines

Updates

Updated test scripts
@ByteMeAsap ByteMeAsap force-pushed the 17.0-add-delivery_sendcloud branch from d809bbd to 60f7cef Compare December 10, 2024 07:05
@ByteMeAsap
Copy link
Author

@rousseldenis , please review

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

Successfully merging this pull request may close these issues.

4 participants