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.

Sign up for GitHub

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

Jump to bottom

[15.0][ADD] hr_announcement: New module #1262

Merged
merged 1 commit into from
Nov 7, 2023
Merged

[15.0][ADD] hr_announcement: New module #1262

merged 1 commit into from
Nov 7, 2023

Conversation

pilarvargas-tecnativa
Copy link
Contributor

The hr_announcement module extends the announcement module by adding 3 new options to the announcement type. This module allows the creation and management of announcements, which can be sent to specific employees, departments, or job positions.

cc @Tecnativa TT42581

@victoralmau @chienandalu please review :)

ping @CarlosRoca13

chienandalu
chienandalu suggested changes Jun 29, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Code review

hr_announcement/models/announcement.py Outdated Show resolved Hide resolved
hr_announcement/models/announcement.py Show resolved Hide resolved
hr_announcement/models/announcement.py Outdated Show resolved Hide resolved
hr_announcement/models/announcement.py Outdated Show resolved Hide resolved
hr_announcement/models/announcement.py Outdated Show resolved Hide resolved
hr_announcement/models/res_users.py Outdated Show resolved Hide resolved
hr_announcement/models/res_users.py Outdated Show resolved Hide resolved
hr_announcement/models/res_users.py Outdated Show resolved Hide resolved
hr_announcement/models/res_users.py Outdated Show resolved Hide resolved
hr_announcement/readme/USAGE.rst Show resolved Hide resolved
@pilarvargas-tecnativa
Copy link
Contributor Author

Code review

Thank you for your review above, I have already made the relevant changes. Can you check again if everything is correct?

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 15.0-add-hr_announcement branch 2 times, most recently from 0e713a5 to 79e1f2c Compare July 14, 2023 11:07
augustodinizl
augustodinizl approved these changes Jul 18, 2023
View reviewed changes
Copy link

@augustodinizl augustodinizl left a comment

Choose a reason for hiding this comment

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

Functional tests on runboat - OK - LGTM
Captura de Tela 2023-07-18 às 18 36 06
Captura de Tela 2023-07-18 às 18 35 52

Captura de Tela 2023-07-18 às 18 37 22
Captura de Tela 2023-07-18 às 18 36 46
Captura de Tela 2023-07-18 às 18 36 29

zamberjo
zamberjo suggested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@zamberjo zamberjo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Functionally it seems to work well. Only a few details

hr_announcement/models/__init__.py Outdated Show resolved Hide resolved
hr_announcement/models/announcement.py Outdated Show resolved Hide resolved
hr_announcement/models/announcement.py Outdated Show resolved Hide resolved
zamberjo
zamberjo approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@zamberjo zamberjo left a comment

Choose a reason for hiding this comment

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

The super that @chienandalu points out is missing, otherwise it looks good.

@pilarvargas-tecnativa
Copy link
Contributor Author

The super that @chienandalu points out is missing, otherwise it looks good.

thanks!!!

@rvalyi
Copy link
Member

rvalyi commented Jul 24, 2023 •
edited
Loading

Hello, yesterday I tried the announcement module on the v15 Runboat and got some issues: basically as the admin I sent an announcement for the demo user and the I had a demo user session in an anonymous browser window.

Problem: the demo user didn't get the notification in the tray unless he reloaded the page. Also I got polling XHR timeouts. Also for the admin user he got the notification in the tray when the announcement was for him, but he had to refresh the page to get the popup. I don't know if this is may be related to the Runboat environment (possibly creating the XHR polling timeouts) or if I did something wrong, but I think this is worth a check on your side.

@pedrobaeza
Copy link
Member

@rvalyi bus is not working in runboats unless you put https on your URLs.

@rvalyi
Copy link
Member

rvalyi commented Jul 24, 2023

@rvalyi bus is not working in runboats unless you put https on your URLs.

ah ok thank you for clarifying.

@pedrobaeza
Copy link
Member

AFAIK, you can type https:// in your browser when using a runboat instance, and a self-signed certificate will arise. Accepting the warning, you can then try it.

gurneyalex
gurneyalex approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@gurneyalex gurneyalex left a comment

Choose a reason for hiding this comment

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

Looks good. An automated test would make it great.

chienandalu
chienandalu approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Working in prod for months. You can mention the lack of tests in the roadmap though

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-1262-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 88e480e into OCA:15.0 Nov 7, 2023
4 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 15.0-add-hr_announcement branch November 7, 2023 10:49
@pedrobaeza pedrobaeza added this to the 15.0 milestone Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gurneyalex gurneyalex gurneyalex approved these changes

@zamberjo zamberjo zamberjo approved these changes

@chienandalu chienandalu chienandalu approved these changes

@augustodinizl augustodinizl augustodinizl approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
15.0
Development

Successfully merging this pull request may close these issues.
8 participants
@pilarvargas-tecnativa @rvalyi @pedrobaeza @OCA-git-bot @gurneyalex @zamberjo @chienandalu @augustodinizl