Skip to content
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

[16.0][MIG] web_notify: Migration to 16.0 #2412

Merged
merged 50 commits into from
Apr 19, 2023

Conversation

baimont
Copy link
Contributor

@baimont baimont commented Feb 17, 2023

#2309

Tested locally using the Test web notify tab on users

@baimont baimont force-pushed the 16.0-mig-web_notify branch from 4a702c5 to d0dd193 Compare February 17, 2023 09:39
Copy link
Member

@FrancoMaxime FrancoMaxime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@baimont baimont force-pushed the 16.0-mig-web_notify branch 2 times, most recently from 7519689 to d81d637 Compare February 17, 2023 13:23
Copy link
Contributor

@sbejaoui sbejaoui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional test.

@tarteo
Copy link
Member

tarteo commented Feb 22, 2023

/ocabot migration 16.0

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Feb 22, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Feb 22, 2023
46 tasks
@alexis-via
Copy link
Contributor

While testing web_notify in v16 with the module currency_old_rate_notify, I found 2 problems:

  1. in v14, it was working well when "message" contained HTML. In v16, the HTML code of "message" is displayed as text !
    notify_html_bug

  2. if target is not set, then the message is displayed to self.env.user and not to the user on which the method was called !!!

In the method _notify_channel(), you have:

if not target:
            target = self.env.user.partner_id

it should be:

if not target:
            target = self.partner_id

If you don't accept this change, you need to make it very clear in the documentation that, if you don't use the target argument, the message is NOT sent to the user on which you called the method, but to the current user!!!

@baimont
Copy link
Contributor Author

baimont commented Feb 28, 2023

  1. if target is not set, then the message is displayed to self.env.user and not to the user on which the method was called !!!

In the method _notify_channel(), you have:

if not target:
            target = self.env.user.partner_id

it should be:

if not target:
            target = self.partner_id

If you don't accept this change, you need to make it very clear in the documentation that, if you don't use the target argument, the message is NOT sent to the user on which you called the method, but to the current user!!!

Done

@baimont
Copy link
Contributor Author

baimont commented Feb 28, 2023

  1. in v14, it was working well when "message" contained HTML. In v16, the HTML code of "message" is displayed as text !
    notify_html_bug

Concerning this, I believe it is due to the t-out attribute here https://github.com/odoo/odoo/blob/d594069767cb0f48c829a25f33abc6da620e7235/addons/web/static/src/core/notifications/notification.xml#L12

But as I am not familiar with owl I'm not sure what's a good way to fix this. If anyone has an idea, you're welcome fixing it with a new commit.

@pedrobaeza
Copy link
Member

/ocabot migration web_notify

Copy link
Contributor

@nguyenminhchien nguyenminhchien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nguyenminhchien
Copy link
Contributor

  1. in v14, it was working well when "message" contained HTML. In v16, the HTML code of "message" is displayed as text !
    notify_html_bug

Concerning this, I believe it is due to the t-out attribute here https://github.com/odoo/odoo/blob/d594069767cb0f48c829a25f33abc6da620e7235/addons/web/static/src/core/notifications/notification.xml#L12

But as I am not familiar with owl I'm not sure what's a good way to fix this. If anyone has an idea, you're welcome fixing it with a new commit.

Hello @baimont i just created this PR to help you show a html formatted message.
Please review it.
image

@baimont
Copy link
Contributor Author

baimont commented Apr 11, 2023

  1. in v14, it was working well when "message" contained HTML. In v16, the HTML code of "message" is displayed as text !
    notify_html_bug

Concerning this, I believe it is due to the t-out attribute here https://github.com/odoo/odoo/blob/d594069767cb0f48c829a25f33abc6da620e7235/addons/web/static/src/core/notifications/notification.xml#L12
But as I am not familiar with owl I'm not sure what's a good way to fix this. If anyone has an idea, you're welcome fixing it with a new commit.

Hello @baimont i just created this PR to help you show a html formatted message. Please review it. image

Thanks very much @nguyenminhchien ! Tested and approved

lmignon and others added 10 commits April 11, 2023 09:53
This technical module allows you to send instant notification messages from the server to the user in live.
Fix a check when comparing a user count with items within a mock call.

The previous method was succeeding by pure luck because OCA test
databases contain 2 users, which happens to be the amount of items
within a mock "call_args" (it contains args + kwargs).
- Use the 'session' class of the JS Framework (session no lounger bound
to web client)
- Test change: compare emitted & received messages based on content, not
order. Using string comparison raises false positives.
Currently translated at 100,0% (5 of 5 strings)

Translation: web-11.0/web-11.0-web_notify
Translate-URL: https://translation.odoo-community.org/projects/web-11-0/web-11-0-web_notify/pt_BR/
Currently translated at 40.0% (2 of 5 strings)

Translation: web-11.0/web-11.0-web_notify
Translate-URL: https://translation.odoo-community.org/projects/web-11-0/web-11-0-web_notify/da/
@baimont baimont force-pushed the 16.0-mig-web_notify branch 2 times, most recently from 785a4fc to 4723fa3 Compare April 11, 2023 07:56
@baimont baimont force-pushed the 16.0-mig-web_notify branch from 4723fa3 to f099779 Compare April 11, 2023 08:03
@marielejeune
Copy link

Hi,
Thanks for the work.
There is an issue: notifications are not popping as soon as we use workers.

@sbidoul
Copy link
Member

sbidoul commented Apr 19, 2023

There is an issue: notifications are not popping as soon as we use workers.

After further investigation with @marielejeune we found the issue was not related to workers but to Chrome blocking web socket due to an invalid certificate. So this should be good to go.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-2412-by-sbidoul-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b05f180 into OCA:16.0 Apr 19, 2023
@OCA-git-bot
Copy link
Contributor

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

@sbidoul sbidoul deleted the 16.0-mig-web_notify branch April 19, 2023 10:13
@pedrobaeza
Copy link
Member

@sbidoul remember that you enabled a runboat certificate (changing http to https) for these cases 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.