-
Notifications
You must be signed in to change notification settings - Fork 23.1k
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
[IMP] portal, mail,...: Use backend chatter in portal #138233
base: master
Are you sure you want to change the base?
[IMP] portal, mail,...: Use backend chatter in portal #138233
Conversation
45a3eb9
to
64a6743
Compare
64a6743
to
e327ec0
Compare
1c14d71
to
93b48a3
Compare
0e4da69
to
4571ebb
Compare
from odoo.addons.portal.controllers.mail import _check_special_access | ||
|
||
|
||
def check_portal_access(func): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably avoid this kind of construct, as it makes code really hard to follow and understand, especially adding magic parameters. Why don't you improve the environment instead ? It seems to be the right place to store that kind of information, along user, lang, ... it seems more inlined with current code, in order to avoid this magic stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @mart-e as it might interest you: isn't it the kind of stuff that should be done cleanly on environment ?
55306f4
to
522cae0
Compare
08fa636
to
bdcc93c
Compare
f9af2b2
to
35ca897
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I had a quick look again, not a complete review (especially not the security part) but I wanted to get a quick idea of the current diff and I had a few comments that came out of it.
cannedResponseIds: composer.cannedResponses.map((c) => c.id), | ||
parentId: this.props.messageToReplyTo?.message?.id, | ||
}; | ||
const postData = this.postData(composer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be composer.postData()
and use this
inside the function
await this._sendMessage(value, postData); | ||
}); | ||
} | ||
|
||
postData(composer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name looks like the function will post data, maybe getPostData
?
@@ -14,7 +14,7 @@ | |||
'p-3': normal, | |||
'o-hasSelfAvatar': !env.inChatWindow and thread, | |||
}" t-attf-class="{{ props.className }}"> | |||
<div class="o-mail-Composer-sidebarMain flex-shrink-0" t-if="!compact and props.sidebar"> | |||
<div class="o-mail-Composer-sidebarMain flex-shrink-0" t-if="!compact and props.sidebar" name="sidebar main"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name is not really necessary in this case, it could target the class o-mail-Composer-sidebarMain
.
@@ -63,6 +63,7 @@ | |||
</div> | |||
<div | |||
class="position-relative d-flex" | |||
name="message content" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We often don't pay too much attention to that, but technically (in HTML) name is not a valid attribute for div
elements.
Maybe we should have a specific class instead, or for custom data we should use data-
something.
# Only messages from the current user not OdooBot | ||
messages = self.out_invoice.message_ids.filtered(lambda msg: msg.author_id == self.partner_a) | ||
self.assertEqual(len(messages), 3) | ||
self.assertEqual(messages[0].body, "<p>test message 3</p>") | ||
self.assertEqual(len(messages[0].attachment_ids), 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the test changed? Where the extra message (from odoo bot) coming from compared to before the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From _message_post_after_hook()
which is called by message_post
. The previous method was portal_chatter_post
without calling _message_post_after_hook()
@@ -6,7 +6,7 @@ | |||
<Message message="props.thread.livechatWelcomeMessage" hasActions="false" thread="props.thread" className="'px-3'"/> | |||
</div> | |||
</xpath> | |||
<xpath expr="//*[@name='content']" position="after"> | |||
<xpath expr="(//div[hasclass('o-mail-Thread')])[last()]" position="after"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't reproduce it now, but I remember there was a bug in isTyping
when there were no content on the thread. I see the typing part is changed in livechat already.
@@ -45,7 +45,7 @@ export function onExternalClick(refName, cb) { | |||
let downTarget, upTarget; | |||
const ref = useRef(refName); | |||
function onClick(ev) { | |||
if (ref.el && !ref.el.contains(ev.target)) { | |||
if (ref.el && !ref.el.contains(ev.composedPath()[0])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a bug here? Can't it be fixed separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bug is introduced in this PR when we have three overlays together on the page, and we want that external click knows exactly which overlay should be affected and closed. I can make a separate fix, but I can't reproduce it there.
'mail/static/src/discuss/typing/**/*', | ||
'mail/static/src/discuss/core/common/action_panel.*', | ||
], | ||
'portal.assets_embed': [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is embed portal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bundle that provides the shadow dom's requirements.
</div> | ||
</xpath> | ||
</t> | ||
</templates> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing new line
// The popup composer still works with public widget and till the user refreshs the page, the comment's change is not visible. | ||
// Todo: Uncomment it when the popup composer would be converted to a component | ||
//{ | ||
// shadow_dom: "#chatterRoot", | ||
// trigger: '.o-mail-Message-content:contains("This is a great course. Top !")', | ||
// run: function () {}, // check review is correctly added | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an intended comment with Todo for the next PR, when we're going to replace and delete old portal rating composer as well. This part of the test should be reverted when it's possible to fetch after posting via new rating composer in elearning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello 👋 Two comments without checking the purpose or checking the code:
-
This represents a 39% increase of frontend JS bundle when only website is installed. Something has to be done: probably lazy loading what is required when the feature is used.
-
We have multiple work planned regarding those JS bundle, one currently being done is removing jQuery (this is being done through multiple (India) PR, one step at a time, we'll ping the relevant teams when we are confident each step is done properly). If you are to introduce that much of an increase of frontend JS, lazy loaded or not, it would be nice if it could not contain any jQuery already 🙏
Thanks 🙏
35ca897
to
6474f6f
Compare
By this commit: - Portal chatter widget is deleted - The chatter in portal is the backend Chatter component (from mail module) now - Some features in chatter like link preview and reactions are new in portal environment (number of queries in some tests are increased because of reactions feature on messages) - Except of two use cases of portal composer (as a popup composer) all the other use cases of composer in portal, are replaced by composer component in mail module - The project sharing doesn't use the previous chatter component in the sharing webclient view. It's replaced by backend chatter component as well task-2828744
Remove authentication via rpc calls and move all security parameters to the kwargs parameters
6474f6f
to
cd77fa9
Compare
By this commit:
P.S.
Task-2828744
Related enterprise PR
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr