[FIX] mail: splicing schedule method #160069

somu-odoo · 2024-04-01T12:09:49Z

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:
This PR separates the schedule method from scheduleActivity method, so that it can be overridden.
Needed to be done to trigger reloadParentView method for documents.document.

See here: odoo-dev/enterprise@14c7a95

Task - 3786861

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

robodoo · 2024-04-01T12:10:05Z

.

pyduf

Hello @somu-odoo,

Just added a small comment about the "Mark as Done" that is not working anymore because it is still calling the props onUpdate instead of the method.

pyduf · 2024-05-13T13:45:30Z

addons/mail/static/src/web/activity/activity.js

@@ -72,6 +72,10 @@ export class Activity extends Component {
        return computeDelay(this.props.data.date_deadline);
    }

+    get onUpdate() {


To support updating the main view when using the "mark as done" button, you should also use this function in onClickMarkAsDone.

async onClickMarkAsDone(ev) { // ... this.popover.open(ev.currentTarget, { // ... reload: this.onUpdate, // (instead of this.props.onUpdate) }); }

Oh my bad, might have missed this one.

alexkuhn · 2024-05-15T08:32:16Z

It's not great to introduce so many artificial splits to enable so much overrides. This makes refactoring the code far more complicated than necessary.

Considering that documents.document override is essentially about doing props.reloadParentView() after many actions, the footprint of override could be much lower if that the original code has a single state flag to determine whether an action triggers a reloadParentView. By default it doesn't, but in documents.document override could be a single LOC that just set this flag to true.

somu-odoo · 2024-05-15T11:59:38Z

Hi @alexkuhn
I do agree that making such artificial splits will introduce a slight complication with code refactoring, but even if I go with your approach of setting a flag after every necessary action called from documents.document, it will need a trigger for the reloadParentView since activity doesn't has an inbuilt way to call the method on it's own. And that will end up being something similar to current one. I hope I understood the approach suggested correctly, if you meant something different, please let me know.

alexkuhn · 2024-05-15T12:56:19Z

@somu-odoo You could have the flag in the env, so that all children components are aware of this flag, including Activity.

The gain is to reduce drastically the footprint of patches / overrides. This is desirable because every patch / override makes the code much less flexible due to them adding feature on top of existing components by relying on their current code structure. The shape of code is mostly arbitrary, so reducing its flexibility can make refactoring very hard. Also in general, the maintainer of the original component is not necessarily the same as the maintainer of the patch, so that's another reason to reduce drastically the work of the patcher if any is necessary.

I get that in the past we went a lot with patching and overriding stuffs in JS, but we learned that they worsen code maintenance. Ideally all features are available in original code and you just have to use the component. Customising the component in an non-official way like with patching / overriding methods should be very rare and minimal.

somu-odoo · 2024-05-16T07:13:24Z

@alexkuhn I've pushed another approach which should work as you have suggested, if I understood it correct. Let me know if this needs any changes

alexkuhn · 2024-05-16T08:23:45Z

addons/mail/static/src/web/chatter.js

+        useEffect(
+            () => {
+                if (this.state.reloadParentView) {
+                    this.reloadParentView();
+                }
+            },
+            () => [this.state.reloadParentView]
+        );


It doesn't look necessary to make a state.reloadParentView and have this useEffect.
You could use this.reloadParentView() like before, but make this function available in useChildSubEnv. In documents override, it just has to this.env.reloadParentView()

This setup made sense since we wished to use flag, but if we omit that, then we can also use this.props.reloadParentView() in our patch, that would remove the need for adding it in env unless we wish to remove that prop as well

Either I miss something or using this.state.reloadParentView is just a complicated way to just this.reloadParentView().

Using a flag in state, so that it triggers a useEffect that actually reloadParentView()... This looks complicated for the sake of being complicated.

The only part that may require something like this is if you have 2 triggers of the flag and you rely on useEffect to result in a single reloadParentView() rather than 2... Is it what you try to solve? If not, then I see no point to make the code more complicated than necessary

Alright, I understand the reason behind removing the flag.
I've pushed the changes as suggested.
Now, since we added the method in env, should we remove the method from props and adapt accordingly? or keep them both for older patches to work properly?

We could, but it's best to do that in master. This is a fix in 16.3 so we should be cautious not to break community code like OCA modules, which could rely on this.props.reloadParentView.

Alright in that case, I have implemented it and have updated both branches accordingly

alexkuhn · 2024-05-16T08:24:48Z

@alexkuhn I've pushed another approach which should work as you have suggested, if I understood it correct. Let me know if this needs any changes

This is much better! I like the reduced code in the patch. Thanks :)

alexkuhn

LGTM

alexkuhn · 2024-05-16T12:38:59Z

addons/mail/static/src/web/chatter.js

+    async schedule(threadId) {
+        await this.activityService.schedule(this.props.threadModel, threadId);
+        this.load(this.props.threadId, ["activities", "messages"]);
+    }


Note for master / 17.3: this.activityService.schedule() has been replaced by this.store.scheduleActivity()

(fyi, reasoning was that discuss code looked very complex with many services, so we killed most of them)

addons/mail/static/src/web/activity/activity.js

This commit modifies childSubEnv to add a support for reloadParentView. Two methods are splitted in order to allow some additional modifications - schedule - onUpdate Implementation of these splits can be seen here: odoo-dev/enterprise@767b8b0 Task - 3786861

C3POdoo added the RD research & development, internal work label Apr 1, 2024

somu-odoo force-pushed the saas-16.3-fix-record-activity-not-updating-somu branch from 203a0c4 to d3bb1fd Compare April 8, 2024 07:20

somu-odoo force-pushed the saas-16.3-fix-record-activity-not-updating-somu branch from d3bb1fd to 92a45ab Compare May 1, 2024 07:03

pyduf reviewed May 13, 2024

View reviewed changes

somu-odoo force-pushed the saas-16.3-fix-record-activity-not-updating-somu branch from 92a45ab to d43e86f Compare May 14, 2024 13:58

somu-odoo marked this pull request as ready for review May 15, 2024 05:03

C3POdoo requested review from a team May 15, 2024 05:05

somu-odoo force-pushed the saas-16.3-fix-record-activity-not-updating-somu branch 2 times, most recently from f3cb15a to 52c9074 Compare May 16, 2024 07:01

alexkuhn reviewed May 16, 2024

View reviewed changes

somu-odoo force-pushed the saas-16.3-fix-record-activity-not-updating-somu branch 2 times, most recently from efaeeb4 to 8252b64 Compare May 16, 2024 10:26

alexkuhn approved these changes May 16, 2024

View reviewed changes

alexkuhn reviewed May 16, 2024

View reviewed changes

addons/mail/static/src/web/activity/activity.js Outdated Show resolved Hide resolved

somu-odoo force-pushed the saas-16.3-fix-record-activity-not-updating-somu branch from 8252b64 to 772faea Compare May 16, 2024 13:04

pyduf approved these changes May 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] mail: splicing schedule method #160069

[FIX] mail: splicing schedule method #160069

somu-odoo commented Apr 1, 2024

robodoo commented Apr 1, 2024 •

edited

pyduf left a comment

pyduf May 13, 2024

somu-odoo May 14, 2024

alexkuhn commented May 15, 2024 •

edited

somu-odoo commented May 15, 2024

alexkuhn commented May 15, 2024

somu-odoo commented May 16, 2024

alexkuhn May 16, 2024

somu-odoo May 16, 2024

alexkuhn May 16, 2024 •

edited

somu-odoo May 16, 2024

alexkuhn May 16, 2024

somu-odoo May 16, 2024

alexkuhn commented May 16, 2024

alexkuhn left a comment

alexkuhn May 16, 2024

[FIX] mail: splicing schedule method #160069

Are you sure you want to change the base?

[FIX] mail: splicing schedule method #160069

Conversation

somu-odoo commented Apr 1, 2024

robodoo commented Apr 1, 2024 • edited

pyduf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkuhn commented May 15, 2024 • edited

somu-odoo commented May 15, 2024

alexkuhn commented May 15, 2024

somu-odoo commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkuhn May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkuhn commented May 16, 2024

alexkuhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robodoo commented Apr 1, 2024 •

edited

alexkuhn commented May 15, 2024 •

edited

alexkuhn May 16, 2024 •

edited