-
-
Notifications
You must be signed in to change notification settings - Fork 63
[16.0][ADD] billcom_integration #166
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
base: 16.0
Are you sure you want to change the base?
Conversation
79b4d72 to
a4451ab
Compare
|
To improve clarity, I would like to propose renaming the addon to billcom_integration. The current technical name, billcom, is quite generic and doesn’t clearly communicate the purpose of the module. Using billcom_integration makes it immediately evident that the addon provides an integration with the Bill.com platform. |
a4451ab to
1c095f8
Compare
a40928e to
594a948
Compare
594a948 to
67825f7
Compare
|
@rrebollo @jelenapoblet @crrodrigueztrujillo guys, reviews needed!!! |
rrebollo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the code looks good. It is a huge effort and a valuable asset to contribute to the OCA ecosystem.
Most of my comments are just suggestions; only a minority require action (clarification or small fixes).
However, looking ahead, I am worried about the size of this addon. Maintenance could become a nightmare, and I think we need a strategy to make this manageable. The sheer effort required to review this would be a dealbreaker for the vast majority of the community unless there is an economic incentive to maintain the addon in its current form.
Maybe it can be split, or perhaps the logic could be encapsulated in a library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ) | ||
| ) | ||
| if move: | ||
| move.with_context(skip_billcom_sync=True).write({"active": False}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about posted moves? Is it valid to archive a posted moves?
| ) | ||
| ) | ||
| if move: | ||
| move.with_context(skip_billcom_sync=True).write({"active": False}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| move.with_context(skip_billcom_sync=True).write({"active": False}) | |
| move.with_context(skip_billcom_sync=True).action_archive() |
| ) | ||
| ) | ||
| if move: | ||
| move.with_context(skip_billcom_sync=True).write({"active": True}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| move.with_context(skip_billcom_sync=True).write({"active": True}) | |
| move.with_context(skip_billcom_sync=True).action_unarchive() |
| ) | ||
| ) | ||
| if partner: | ||
| partner.with_context(skip_billcom_sync=True).write({"active": False}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| partner.with_context(skip_billcom_sync=True).write({"active": False}) | |
| partner.with_context(skip_billcom_sync=True).action_archive() |
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class BillcomWebhookLog(models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Webhooks are a built-in (limited feateure in odoo 17.0), thinking in migration. you can checkout also OCA/webhook#13 OCA/webhook#14
| if self.child_ids: | ||
| for child in self.child_ids: | ||
| if child.type == "invoice": | ||
| billing_address = child | ||
| elif child.type == "delivery": | ||
| shipping_address = child |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if self.child_ids: | |
| for child in self.child_ids: | |
| if child.type == "invoice": | |
| billing_address = child | |
| elif child.type == "delivery": | |
| shipping_address = child | |
| for child in self.child_ids: | |
| if child.type == "invoice": | |
| billing_address = child | |
| elif child.type == "delivery": | |
| shipping_address = child |
A partner could have multiple invoice and delivery address so ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide instructions for configuring the initial API connection, including authentication methods and required credentials.
Add also details on the specific webhooks that need to be configured in your Bill.com account to enable real-time data synchronization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any multicompany rule. Is this intended?
| billcom_partner_phone = fields.Char(string="Bill.com Phone", readonly=True) | ||
|
|
||
| # Multi-currency duplicate handling | ||
| billcom_ids_json = fields.Text( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you storing a json ? why don't use fields.Binary?
This is a valid POV, we need to consider long term maintenance for this integration, it's already in v16 , what about migrations to v17 and v18 ? |
|
To make this more manageable and more “OCA-friendly” long term, we should reduce the surface area of each module. As we already discussed, the right way forward is to split the integration into: billcom_integration_base
billcom_integration_customers
billcom_integration_vendors
This split will make review and testing more approachable, let users install only what they need, keep the base addon reusable, and lower the maintenance burden in the future. I’m happy to help refine the boundaries between modules if we move in this direction, but I strongly recommend not merging this as a single, very large addon. |

@BinhexTeam T17939