[18.0][MIG] web_send_message_popup: Migration to 18.0

[Du-ma]

Module migration from 16.0 straight to 18.0, cba with 17.0.
Made sure pre-commit doesn't give warnings on import sorting (unlike some modules...).

Massive .js changes, previous version (16.0) .js code was absolutely incompatible.

[gaikaz]

@OCA/web-maintainers
Could this PR get some love, please 🙏🏻

[tarteo]

Is it not possible to change the t-on-click of the "Send message" button? And use the Composer component directly instead of copying the functions to the Chatter?

[Du-ma]

@tarteo
Yes to both, HOWEVER!

• IMO there's no point in changing the t-on-click function, but just to make it confusing.
• Using the composer directly would require some weird prototyping and maybe catching the existing instance of it (if were being clean), to the point where simplicity would overshadow the complexity.

In all seriousness I see where you're going with this, but no.
Unless you have something in mind that I may have missed?
Do elaborate if that's the case.

[tarteo]

After some digging, what you can do is create a callback prop and pass the onClickFullComposer to expose it to the chatter. something like this in the setup function:

if (this.props.someCallback) {
   this.props.someCallback({onClickFullComposer: this.onClickFullComposer});
}

[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). 🤖

[Du-ma]

@tarteo
Over my dead body

[Du-ma]

@dreispt
Would it be possible to merge this? 🙏

[Du-ma]

@pedrobaeza 🥺

[pedrobaeza]

@tarteo Over my dead body

This seems a very aggressive language, don't you think?

Reading the thread, it seems there are suggestions to improve the code. Have they been considered?

[Du-ma]

@pedrobaeza @tarteo
A joke, cmon... 😕, it wasn't supposed to sound aggressive...
The suggestion would allow for less lines of code, that's true!
But that would require doing something unorthodox, there's a reason we would need to mutate the thing through props, and in setup too - you're not supposed to do it like that.
Look, I'm all game for less lines of code, but not when it's at the cost of readability or bad practices, plus it's not like what I got down is worse.
There's comments where the functions are from and stuff, so when someone (including me) is migrating this module they know exactly where to look and what to look out for.

But at the end of the day, if merging this means doing that, well... I don't have a choice I guess.

[pedrobaeza]

Glad to hear that it was just a joke. I think most of us thought otherwise. The written language + the lack of context makes it very easily misunderstood, so please take it into account for other times in the future.

Thanks for the extra explanation about why not changing it. I lack a bit of knowledge in JS, but seems reasonable. What you have to do in that changes, is to put a comment over the code stating that reasons for doing it that way, as they seem not obvious - don't do that with every obvious thing - so other developers looking at the code (or you future you) remember why it was done that way and why a refactoring "reducing" code is not convenient, so you avoid the temptation next time of this being proposed to be changed again.

Can you please add such comment and we proceed with the merge?

[tarteo]

I already suspected it was a cultural difference or language barrier. What I'm suggesting is not unorthodox, it's a common way to do it: https://github.com/odoo/owl/blob/master/doc/reference/props.md#binding-function-props It's even proposed if you use deprecated t-ref attribute:

OwlError: t-ref is no longer supported on components. Consider exposing only the public part of the component's API through a callback prop.

[tarteo]

But I'm fine merging it with the clarifying comments

[Du-ma]

@tarteo
Nah im just stupid, srr I just thought it was funny at the time, mb 😕
When I read it back ig it's rly not that funny...

And you're right about callback methods, they're fine!
But keep in mind we would need to like not only do a callback but mutate the parent itself through the callback, that's the part I don't like... 😕
Since like everything needs to happen on toggleComposer.
If I was to do it, I would pass the callback method in props that THEN would pass onClickFullComposer method back from Composer to the Chatter and we then modify this (Chatter) with a function to use for later in toggleComposer.
And when all that is done, were calling methods from a Component that is allegedly inactive 😕

It's just a bunch of forth and back, it can get confusing and it doesn't sound right...
If it was 1 dimensional where we use the passed callback and were all g - I would be all game man!

@pedrobaeza
Done!

[tarteo]

/ocabot merge nobump

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-3156-by-tarteo-bump-nobump, awaiting test results.

[pedrobaeza]

Thanks everybody for the constructive technical debate!

[pedrobaeza]

/ocabot migration web_send_message_popup

[OCA-git-bot]

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