web_notify: migration to 11.0 #781
Conversation
|
Marking as "Work in progress" until you tell me about the JS. |
|
Removed the Note: not completely sure why the polling is started in However this is a migration, not a refactoring - if it ain't broken, don't fix it. Unless you want me to. |
|
@lmignon you were the original author. Maybe you can clarify @dbo-odoo's comments. |
|
@pedrobaeza I'm not at work this week. I'll take a look at tis PR the next week |
|
@yajo |
|
@yajo said
I'm gonna have to disappoint you, since I'm not part of the JS Framework team, I'm not really familiar with the new webclient architecture (neither the new one nor the old one)... I squashed the translations commits and moved the error map to a method as requested :) |
|
Small thing to notice: I'm unable to make this work in the runbot (tests work but a manual test does not). I think this might be a runbot server configuration issue because it works fine locally (and in tests) and because the network activity shows some strange things; e.g.: as the response for the |
There was a problem hiding this comment.
@dbo-odoo Thank you for the migration LGTM (code review only). If I remember well, the polling is started into the 'show_application' method to be sure that all the required stuff to show the notifications are in place. It's maybe no more the most appropriate place but if it works I agree with you that we can leave it there.
web_notify/models/res_users.py
Outdated
| @@ -0,0 +1,46 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
In python3 # -*- coding: utf-8 -*- it should be removed.
Same for the others python file in this pull request.
There was a problem hiding this comment.
It's not strictly needed, but optional when you have already encoded your file.
|
@dbo-odoo thanks for the migration. I hope you can find time to take into account the last few comments ;) |
This technical module allows you to send instant notification messages from the server to the user in live.
…orth additional database columns.
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.
|
@jcdrubay I forgot about this, thanks for the reminder :) Commits squashed, encoding removed as per @phatnguyenuit's suggestion. Hope you all had a nice easter weekend 🐇 🥚 (assuming you're somewhere it gets celebrated, otherwise I hope you had a nice completely normal weekend :p) Cheers! |
|
@yajo Your comments have been taken into account :) Can you approve? Maybe merge? Thanks |
Still have to check the js code to see if everything is still needed.