[12.0][MIG] web_notify #1071

aitorbouzas · 2018-10-10T08:42:53Z

Migrated and tested. Minor changes in JS.

Messed up the commit history in #1070
CC for help with JS/code review: @lmignon @JayVora-SerpentCS

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/

@hbrunn

Only the admin user (sudo) is allowed to send notifications to other users. The normal users can only send notifications to themselves. This is to prevent attackers to craft malicious notifications and send them to other users using RPC. Correction based on the idea of @hbrunn

lmignon

LGTM (Code review only)

pedrobaeza · 2018-10-11T22:13:04Z

Maybe we can add as demo data a button (or 2, one for each kind of notification) on user form for launching a default notification, which can be done using the message as keyword argument, and this way, we can self-test the module. What do you think?

aitorbouzas · 2018-10-11T22:16:01Z

@pedrobaeza Awesome. Maybe we can show the buttons only when debug mode is active?

pedrobaeza · 2018-10-11T22:17:07Z

Well, if we load them as demo, that extra layer maybe is not needed, but it's up to you to decide which one is best.

nikul-serpentcs

Minor Change

web_notify/__manifest__.py

web_notify/views/web_notify.xml

aitorbouzas · 2018-10-12T11:27:46Z

@pedrobaeza Added the buttons and updated USAGE.rst to show how to test it.
There's only one little problem, the message keyword argument shows the context when using the buttons, why is the context parsed as message?

pedrobaeza · 2018-10-12T16:03:23Z

Uhm, maybe there's an argument mix, but I'm testing on runbot and can't get the notification. I think it's better to put the buttons on user form, not user preferences. Being a popup can also waste a bit.

aitorbouzas · 2018-10-12T17:50:34Z

I'm getting the notifications correctly in my local environment... That's strange, can I check runbot's Odoo log?
Let's see if it works now that I moved the buttons to users form.

aitorbouzas · 2018-10-12T18:13:27Z

Ok, I think I know the reason why it fails, could it be due to have more than 1 worker in runbot?

web_notify/__init__.py

simahawk

LG overall, small remarks

simahawk · 2018-10-15T10:23:24Z

web_notify/models/res_users.py

+            'title': title,
+            'sticky': sticky
+        }
+        notifications = [(getattr(record, channel_name_field), bus_message)


don't use getattr, you can access fields in dict-like notation: (record[channel_name_field], bus_message)

Thanks! It could be also a security problem. Bad habit to use getattr...

simahawk · 2018-10-15T10:24:22Z

web_notify/models/res_users.py

+            'notify_warning_channel_name', message, title, sticky)
+
+    def _notify_channel(self, channel_name_field, message, title, sticky):
+        if (self.env.uid != SUPERUSER_ID


I know this a recent patch but: wouldn't be better to check for a group or to use self.env.user._is_admin()?
/cc @hbrunn

_is_admin

simahawk · 2018-10-15T12:17:28Z

@aitorbouzas seems good: pls, squash your commits :)

aitorbouzas · 2018-10-15T12:39:31Z

@simahawk Squashed!

simahawk · 2018-10-15T12:51:21Z

@aitorbouzas do you really want to keep "Add buttons to users form"? If yes, you must split it as it contains things not related to it.
NOTE: next time when you use "--fixup" you can pass the commit to fixup like:

git commit --fixup 1234
git log
1234 A
4323 B
2343 fixup! A

then when you rebase w/ auto-squash (ie: git rebase -i --autosquash HEAD~3) you'll get them sorted properly.
In your case I'd expect to have a log like:

xxx web_notify: migrate to v11
yyy web_notify: add buttons to users form
zzz web_notify: improve security check

Do not rely on SUPERUSER_ID:
delegate higher permisions check to `user._is_admin()`.

Improves https://github.com/OCA/web/pull/1071/commits/ae8e4ec59deea6063bd912d6be0f62e4e5a8bc3b

kind of... 😉
NOTE2: you can reword commits in the rebase interactive session

Add self-test buttons in demo environment, Updated readme to show how to test it. Add buttons to users form Do not rely on SUPERUSER_ID and avoid getattr usage

aitorbouzas · 2018-10-15T12:57:31Z

@simahawk resquashed commits and reworded some. How's it now?

simahawk · 2018-10-15T13:21:58Z

@JayVora-SerpentCS ok for you?

simahawk · 2018-10-15T13:22:45Z

well, seem so, his comment is attended anyway.
@aitorbouzas thanks for your work!

aitorbouzas · 2018-10-15T13:26:52Z

@simahawk Thank you for the reviews!

JayVora-SerpentCS · 2018-10-31T12:19:13Z

Does it also solve the known issue of it with workers ?

aitorbouzas · 2018-10-31T12:33:53Z

@JayVora-SerpentCS it doesn't, anyway I don't know if workers are the reason of the problem. It was just a supposition. Has this happened to you too?

JayVora-SerpentCS · 2018-11-12T10:35:07Z

Happened once. I have @serpentcs-dev1 check it once.

lmignon and others added 13 commits October 10, 2018 10:40

New module web_notify

f50f17a

This technical module allows you to send instant notification messages from the server to the user in live.

[MIG] Make modules uninstallable

89583d9

[MIG] Migration started web_notify

2fc5ab2

[FIX] Return result of super of show_application

87bd04d

Update web_client.js

e3f44ea

OCA Transbot updated translations from Transifex

497b584

[MIG] web_notify: Migration to 11.0

b7fcfc5

- 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.

[UPD] Update web_notify.pot

b8660e8

Translated using Weblate (Portuguese (Brazil))

58492f3

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/

Translated using Weblate (Danish)

0ad1c90

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/

[UPD] Update web_notify.pot

453f31e

OCA-git-bot mentioned this pull request Oct 10, 2018

Migration to version 12.0 #1048

Closed

40 tasks

pedrobaeza added this to the 12.0 milestone Oct 10, 2018

pedrobaeza added the needs review label Oct 10, 2018

aitorbouzas force-pushed the 12.0-mig-web_notify branch from 534ec53 to 747789f Compare October 10, 2018 12:52

lmignon approved these changes Oct 11, 2018

View reviewed changes

nikul-serpentcs reviewed Oct 12, 2018

View reviewed changes

web_notify/__manifest__.py Show resolved Hide resolved

web_notify/views/web_notify.xml Outdated Show resolved Hide resolved

aitorbouzas force-pushed the 12.0-mig-web_notify branch from 747789f to 1aa6000 Compare October 12, 2018 11:26

aitorbouzas force-pushed the 12.0-mig-web_notify branch from 1aa6000 to 490e571 Compare October 12, 2018 12:01

JayVora-SerpentCS suggested changes Oct 15, 2018

View reviewed changes

web_notify/__init__.py Show resolved Hide resolved

simahawk requested changes Oct 15, 2018

View reviewed changes

aitorbouzas force-pushed the 12.0-mig-web_notify branch from 8ba6c0d to f57d7bf Compare October 15, 2018 12:39

[MIG] web_notify: Migration to 12.0

fbb550f

Add self-test buttons in demo environment, Updated readme to show how to test it. Add buttons to users form Do not rely on SUPERUSER_ID and avoid getattr usage

aitorbouzas force-pushed the 12.0-mig-web_notify branch from f57d7bf to fbb550f Compare October 15, 2018 12:55

simahawk approved these changes Oct 15, 2018

View reviewed changes

simahawk merged commit acc6e87 into OCA:12.0 Oct 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12.0][MIG] web_notify #1071

[12.0][MIG] web_notify #1071

aitorbouzas commented Oct 10, 2018

lmignon left a comment

pedrobaeza commented Oct 11, 2018

aitorbouzas commented Oct 11, 2018

pedrobaeza commented Oct 11, 2018

nikul-serpentcs left a comment

aitorbouzas commented Oct 12, 2018

pedrobaeza commented Oct 12, 2018

aitorbouzas commented Oct 12, 2018

aitorbouzas commented Oct 12, 2018

simahawk left a comment

simahawk Oct 15, 2018

aitorbouzas Oct 15, 2018

simahawk Oct 15, 2018

hbrunn Oct 15, 2018

aitorbouzas Oct 15, 2018

simahawk commented Oct 15, 2018

aitorbouzas commented Oct 15, 2018

simahawk commented Oct 15, 2018 •

edited

Loading

aitorbouzas commented Oct 15, 2018

simahawk commented Oct 15, 2018

simahawk commented Oct 15, 2018

aitorbouzas commented Oct 15, 2018

JayVora-SerpentCS commented Oct 31, 2018

aitorbouzas commented Oct 31, 2018

JayVora-SerpentCS commented Nov 12, 2018

[12.0][MIG] web_notify #1071

[12.0][MIG] web_notify #1071

Conversation

aitorbouzas commented Oct 10, 2018

lmignon left a comment

Choose a reason for hiding this comment

pedrobaeza commented Oct 11, 2018

aitorbouzas commented Oct 11, 2018

pedrobaeza commented Oct 11, 2018

nikul-serpentcs left a comment

Choose a reason for hiding this comment

aitorbouzas commented Oct 12, 2018

pedrobaeza commented Oct 12, 2018

aitorbouzas commented Oct 12, 2018

aitorbouzas commented Oct 12, 2018

simahawk left a comment

Choose a reason for hiding this comment

simahawk Oct 15, 2018

Choose a reason for hiding this comment

aitorbouzas Oct 15, 2018

Choose a reason for hiding this comment

simahawk Oct 15, 2018

Choose a reason for hiding this comment

hbrunn Oct 15, 2018

Choose a reason for hiding this comment

aitorbouzas Oct 15, 2018

Choose a reason for hiding this comment

simahawk commented Oct 15, 2018

aitorbouzas commented Oct 15, 2018

simahawk commented Oct 15, 2018 • edited Loading

aitorbouzas commented Oct 15, 2018

simahawk commented Oct 15, 2018

simahawk commented Oct 15, 2018

aitorbouzas commented Oct 15, 2018

JayVora-SerpentCS commented Oct 31, 2018

aitorbouzas commented Oct 31, 2018

JayVora-SerpentCS commented Nov 12, 2018

simahawk commented Oct 15, 2018 •

edited

Loading