-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[16.0][ADD] attachment_log #3161
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
Conversation
9b18974
to
fc0cd0a
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.
Here are my concerns: why should we post a message for every attachment added?
What if an incoming email arrives with 10+ attachments in it? It will result in 10+ messages trashing the record chatter.
You can simply show name of the user who added attachment together with the creation date. That is.
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 add test coverage for the features you are adding.
7e55b4c
to
81f090d
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.
You need to add CONFIGURE.rts to explain the configuration process.
To enable note logging of attachment operations in record chatter:
- Go to "Settings > General Settings" and scroll to the "Discuss" section.
- Activate the "Attachment Logging" checkbox.
Add screenshots that make both configuration and usage easier. Must be not more than 800px width.
Here is an example. Don't use this picture because you need to fix the configuration view first.
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.
Code review LGTM
Hi @yostashiro! Thank you for noticing that! Will fix. |
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.
Functionality LGTM
Hi @yostashiro could you please check again on the runboat? |
Took a quick look in runboat and noticed a couple of things: 1/ When I attach a file directly to the thread (not via send message or log note), the user and date information doesn't seem to be assigned to the attachment. 2/ The assigned date seems to show in UTC. The best would be to show it in user's timezone. If these are as intended, we may want to update the README to mention these? |
2dea328
to
e9d0365
Compare
@yostashiro Could you please check again? |
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.
Functional test. LGTM with a question.
I'm not sure how to interpret the CI error.
tz = self._context.get("tz") or self.env.user.tz | ||
create_date = self.create_date | ||
create_date = Datetime.context_timestamp( | ||
self.with_context(tz=tz), timestamp=create_date | ||
) |
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 wasn't sure this conversion was necessary on the server side. Does it not work if we just return the raw self.create_date
and use widget="datetime"
in the view?
Merging based on the reviews. |
/ocabot merge nobump |
e9d0365
to
4e3d7c5
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.
I think this is a pretty good feature and tests seems to be failing for different reason. Code and functional review LGTM
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
@ivs-cetmix your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-3161-by-ivs-cetmix-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
@geomer198 Can you rebase de code to retry the tests? |
4e3d7c5
to
9dead3b
Compare
9dead3b
to
7fe2718
Compare
Done! |
@Christian-RB |
@ivs-cetmix Can you retry the merge command? Thanks in advance |
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at c89a36e. Thanks a lot for contributing to OCA. ❤️ |
This module adds the following features for attachments: