Closed
Conversation
qgroulard
reviewed
Nov 18, 2025
qgroulard
left a comment
qgroulard
left a comment
There was a problem hiding this comment.
Hi, this is an attempt at reviewing this PR but:
- I'm not really comfortable with this topic
- I think it's lacking a lot of explanation
To ease the review, could you please:
- Squash your 2 migration commits
- Minimize the diff as much as possible, i.e. push only relevant changes. Please don't remove still accurate docstrings, reword working code, etc. Unless it really improves the code of course.
- Provide explanation on the key changes. Please make use of the "description" section of this PR. Such a big PR should be heavily documented, not pushed with an empty description.
Once this PR is nice and clean, hopefully we can get the help of @hbrunn to get it reviewed and merged 👍
Comment on lines
51
to
55
| Return list of dictionaries [{ | ||
| 'id': sms.sms id, | ||
| 'state': sms.sms#state, | ||
| 'failure_type': sms.sms#failure_type | ||
| 'uuid': sms.sms uuid, | ||
| 'state': sms state, | ||
| 'failure_reason': optional failure reason | ||
| }] |
| ["|", ("company_id", "=", False), ("company_id", "=", self.env.company.id)] | ||
| providers = self.search( | ||
| [ | ||
| ("active", "=", True), |
There was a problem hiding this comment.
I don't think adding ("active", "=", True) has any effect here.
Comment on lines
128
to
129
| sms_records = SmsSms.browse([msg["id"] for msg in messages_to_send]) | ||
| provider_result = gateway._send_via_self(sms_records) or [] |
There was a problem hiding this comment.
Could you please explain why you need to browse the sms record before calling _send_via_self() now ?
Comment on lines
156
to
194
| def _send_partition_providers(self, messages): | ||
| """ | ||
| Return a dict with providers in self as keys and lists of messages a provider | ||
| will handle as value | ||
| ie | ||
| { | ||
| ir.gateway.record(42,): [{message1}, {message2}], | ||
| ir.gateway.record(43,): [{message3}], | ||
| ir.gateway.record(44,): [], | ||
| ir.gateway.record(): [{messages not handled by any available provider}], | ||
| } | ||
| """ | ||
| result = {} | ||
| remaining_messages = messages[:] | ||
|
|
||
| # Sort providers by sequence (priority) | ||
| sorted_providers = self.sorted(key=lambda p: p.sequence) | ||
| _logger.info( | ||
| "Sorted providers by sequence: %s", | ||
| [(p.name, p.sequence) for p in sorted_providers], | ||
| ) | ||
|
|
||
| while remaining_messages: | ||
| message = remaining_messages.pop() | ||
| for this in self: | ||
| if this._can_send(message): | ||
| result.setdefault(this, []).append(message) | ||
| assigned = False | ||
| for provider in sorted_providers: | ||
| can_send = provider._can_send(message) | ||
| _logger.info( | ||
| "Provider %s can send to %s: %s", | ||
| provider.name, | ||
| message.get("number"), | ||
| can_send, | ||
| ) | ||
| if can_send: | ||
| result.setdefault(provider, []).append(message) | ||
| assigned = True | ||
| _logger.info( | ||
| "Assigned message %s to provider %s", | ||
| message.get("id"), | ||
| provider.name, | ||
| ) | ||
| break | ||
| else: | ||
| if not assigned: | ||
| result.setdefault(self.browse([]), []).append(message) | ||
| _logger.info("No provider found for message %s", message.get("id")) | ||
| return result |
There was a problem hiding this comment.
In general, could you please try to minimize the diff by avoiding unnecessary changes.
For instance here I don't understand the change in the docstring.
Also the model ir.sms.gateway is ordered by sequence, thus I don't understand the sort you are adding.
Finally, how is different the use of your variable assigned from the previous for else ?
bjouini-acsone
force-pushed
the
mig-sms_alternative_provider-18.0-bjo
branch
8 times, most recently
from
d4c982b to
e3ff947
Compare
bjouini-acsone
force-pushed
the
mig-sms_alternative_provider-18.0-bjo
branch
from
e3ff947 to
9e1cf38
Compare
Member
|
Overseeded by #360 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this is a partial migration of the Pr:#327 on v16
the major changements from v16 to 18:
the sms_api model does not exist anymore
on the previous implementation , if we found None passede we use the custom logic that we add , but in v18 if we pass None to the send method we always use the standard behaviour of odoo , this is why it is changed in this version