Purchasing shipping labels shouldn't be a side effect of sending a confirmation email.

[artshipulin]

• Impacted versions:
  • 15.0, maybe other
• Steps to reproduce:
  • Develop an override to _send_confirmation_email method of stock.picking that sends emails conditionally.
  • Validate a transfer that is supposed to purchase a shipping label w/o sending a confirmation email.
• Current behavior:
  • Shipping labels are purchased as a side effect of sending confirmation emails, any records filtered out to not send confirmation emails also do not purchase labels.
• Expected behavior:
  • Shipping labels are purchased before sending confirmation emails
  • Not sending confirmation emails does not interfere with purchasing of shipping labels.

It's unintuitive that a method called _send_confirmation_email would also trigger purchasing of shipping labels. With any extension to send confirmation emails conditionally one would also unknowingly disable purchasing of shipping labels. Side effects are bad in general, but such a major function as buying shipping labels really shouldn't be a side effect of a much less important function of sending confirmation emails.

The code that causes this: https://github.com/odoo/odoo/blob/7e4d5c08b201b424deedf684f6a40adc4be448fa/addons/delivery/models/stock_picking.py#L180C9-L183

def _send_confirmation_email(self):
    for pick in self:
        if pick.carrier_id and pick.carrier_id.integration_level == 'rate_and_ship' and pick.picking_type_code != 'incoming' and not pick.carrier_tracking_ref and pick.picking_type_id.print_label:
            pick.sudo().send_to_shipper()

[gdgellatly]

You could write this for loads of functions. This one even makes some sense, no less sense than SMS are also sent as part of same function, it is really just a call after action_done activities. Just write the override differently. Sometimes it isn't possible but for this one it definitely is. Putting in a conditional email cutout is common fare and not usually done at an individual object level but via context key passed to mail objects.

[artshipulin]

@gdgellatly, so if you ordered a coffee, but in addition to the coffee the system also took out a mortgage and sold your kidneys, you would be ok with that? A method called send confirmation email has no business doing anything besides sending a confirmation email. It would make more sense to have a general method that launches all post validation processes (not called send email) which would in turn call relevant methods in correct order (via overrides of course). It's just a bad method name that doesn't describe what the method does. Absolutely every serious software developer with a few years of experience will tell you that this is a code smell and that side effects are (usually) bad. There are thousands of articles and books on the subject of single responsibility principle, side effects and related subjects. I cannot list all of them, but here are a few:

• https://medium.com/thinkster-io/code-smell-side-effects-caf799df2151
• https://luzkan.github.io/smells/side-effects
• https://en.wikipedia.org/wiki/Separation_of_concerns

[gdgellatly]

lol, I'm not saying its right, but you can moan and no one is going to fix it in a stable version where loads of people already have overrides so you need to deal with what is there. But if you want, make a PR changing the function name to _perform_confirmation_activities, or make a PR in master separating it. It is just the way of the world, these things never get fixed in stable, especially a version going EoL in a few months.

[artshipulin]

Well, the only reason I listed 15.0 is because I was working with 15.0 when this came up. I didn't check other versions. Improving code quality going forward should definitely be a thing. It's not true that absolutely insane changes aren't frequently introduced in stable versions that are about to reach EoL. E.g.: 95ac003 or a few months ago when someone tried to fix Werkzeug Unicode handling for some edge cases and broke everything else, or a few more months ago when someone "updated" requirements.txt but only tested on Python 3.6 (which reached EoL several years before) and completely broke it for all other Python versions. Let's not forget that label printing was completely replaced in 15.0 two weeks after official launch...

[gdgellatly]

2 weeks after launch isn't really stable though. Most people wait 3 months min from launch. Lots of crazy changes in every version. As I say, just trying to help, you have myriad options to make work as you need, but amongst the 2600 other issues, short of making a PR I really don't see a community issue going anywhere. Even with PR's and Enterprise tickets it is basically impossible in stable these days. And yes improving quality is a thing, and you clearly have the skills so a PR on master would be much easier and faster.

[artshipulin]

We made it work for ourselves by monkey patching whatever we need. I was just trying to raise awareness of this issue. I think defending or excusing bad practices isn't useful. Getting used to bad practices is also a type of Stockholm syndrome, so I prefer not to do that. With the amount of money and the number of developers Odoo has as a corporation, going through 2600 issues should be about a week worth of work.