[18.0][MIG] iap_alternative_provider: Migration to 18.0

[natuan9]

Change:

• This commit makes the Service field required, so the Service should be set when the Provider is changed rather than when creating the IAP Account

[wlin-kencove]

Wondering if the onchange_provider method would be redundant since i see this a few lines below a method that seems like it would set the service as well.

def write(self, vals):
super().write(vals)
self._set_service_from_provider()
return True

I guess the difference would be that for onchange is that the new service value would be visible on the form before it is saved, where with the write method the new service value would only be set after the record is saved.

It does feel like the _get_service_from_provider() method will be called twice when a provider is changed though, once in the onchange and then another time when saving the record via write().

Is there anyway to avoid that? Was going to suggest removing the overridden write method but seems like we need that in case we update via orm (rpc or script) or any other way besides through the form.

[wlin-kencove]

To avoid redundant calls, can we check the vals dictionary in the write method to see if service value was changed?

If service is to be updated, perhaps we can skip the self._set_service_from_provider() call. The assumption being of course that the onchange would have already set the service field to the correct value from the ui.

[natuan9]

From version 18.0, the Service field is required when creating a new IAP account. Changing the service as soon as the provider is selected will provide a better user experience than randomly selecting a service to create a new IAP account and then having the service updated according to the provider.

As you mentioned, we need this in case we update via ORM (RPC or script). We should not skip the self._set_service_from_provider() call to ensure the service corresponds to the provider.

[wlin-kencove]

@natuan9

Right, i understand that the self._set_service_from_provider() shouldn't be removed from the write(), though I was hoping for a way to reduce unneeded calls.

Something like this:

def write(self, vals):
        res = super().write(vals)
        if "service_id" not in vals:
            self._set_service_from_provider()
        return res

I guess the downside is if a user would update the service id incorrectly and adding checks for that might negate any benefits over just calling self._set_service_from_provider() anyways but just wanted to put that out there.

[natuan9]

Hi @wlin-kencove I have updated, please help to review

[wlin-kencove]

@natuan9

After thinking about it it is probably not necessary to to check for update here.

We are probably okay with how it was before. Sorry for the back and forth, thinking that trying to change the write here will not likely matter in the long run since it will likely call self._set_service_from_provider() anyways.

This update will prevent unneeded calls when service or provider was not change (and thus the call would not be needed) but will not prevent duplicate calls when it is updated via form. It still has value in the first scenario but not sure if that might introduce issues later on. It might be better to just leave as it was and live with the duplicate calls on service/provider update since that will happen anyways.

Sorry again for this diversion.

[natuan9]

No problem at all, I have removed the check

[kobros-tech]

Can we better insert commit of auto pre-commit before the migration commit?

[kobros-tech]

pre-commit auto-fix

[kobros-tech]

I need to understand if the necessity to interchange service_name with service_id?
and if possible to use them both?

[natuan9]

There is no pre-commit auto-fix during migration using oca-port.

In version 17.0, the iap.account model only had the service_name field. In version 18.0, the iap.service model was introduced. The change from service_name to service_id improves data integrity by following a Many2one relationship instead of using a string. Using both would be redundant.

[kobros-tech]

thanks

[hussain]

Functional OK

[hussain]

@kobros-tech can you please review the requested changes? approval is hanging since there is a pending review.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[dannyadair]

Not sure if this belongs into a particular iap_account implementation but regarding _set_service_from_provider() I see a similar issue with the state not being "registered".

In my specific case, the account of an alternative provider (ClickSend) uses (always) the "sms" service.
If the state is not "registered" there will always be the message "You cannot send SMS while your account is not registered" with a link to Odoo registration. That's even shown in the create form on the fly.

So at the moment I have to do this:

from odoo import fields, models, api

class IapAccount(models.Model):
    _inherit = "iap.account"

    provider = fields.Selection(
        selection_add=[("sms_clicksend", "SMS ClickSend")],
        ondelete={"sms_clicksend": "cascade"},
    )
    sms_clicksend_username = fields.Char(string="API Username")
    sms_clicksend_password = fields.Char(string="API Key")

    @api.onchange('provider')
    def _onchange_provider(self):
        """ClickSend accounts are always for SMS and always registered"""
        if self.provider == 'sms_clicksend':
            self.service_id = self.env['iap.service'].search(
                [('technical_name', '=', 'sms')], limit=1
            )
            self.state = 'registered'

    @api.model_create_multi
    def create(self, vals_list):
        """Always treat ClickSend accounts as registered"""
        for vals in vals_list:
            if vals.get('provider') == 'sms_clicksend':
                vals['state'] = 'registered'
        return super().create(vals_list)

    def write(self, vals):
        """Always treat ClickSend accounts as registered"""
        if vals.get('provider') == 'sms_clicksend':
            vals['state'] = 'registered'
        return super().write(vals)

    def _get_account_info(self, account_id, balance, information):
        """Mock account info query for ClickSend"""
        if self.provider == "sms_clicksend":
            # TODO: Query Clicksend API to get the balance
            return {
                'balance': False,
                'warning_threshold': 0.0,
                'state': 'registered',
                'service_locked': True,
            }
        return super()._get_account_info(account_id, balance, information)

and

<?xml version="1.0" encoding="UTF-8" ?>
<odoo>
    <record id="iap_account_view_form" model="ir.ui.view">
        <field name="model">iap.account</field>
        <field name="inherit_id" ref="iap.iap_account_view_form" />
        <field name="arch" type="xml">
            <xpath expr="//group[@name='account']" position="after">
                <group
                    string="ClickSend account"
                    name="clicksend"
                    invisible="provider != 'sms_clicksend'"
                >
                    <field name="service_id" readonly="1" />
                    <field name="sms_clicksend_username" />
                    <field name="sms_clicksend_password" />
                </group>
            </xpath>
        </field>
    </record>
</odoo>

Would be nice if there was a hook for treating an alternative account as registered. In fact, given the odoo.com-centric treatment of that it could probably just be the default to treat alternative accounts as registered.

[dreispt]

/ocabot migration iap_alternative_provider
/ocabot merge nobump

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 18.0-ocabot-merge-pr-3241-by-dreispt-bump-nobump, awaiting test results.

[OCA-git-bot]

It looks like something changed on 18.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 18.0-ocabot-merge-pr-3241-by-dreispt-bump-nobump, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at c0040d5. Thanks a lot for contributing to OCA. ❤️