Skip to content

[add] cms_notification #13

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
Merged
simahawk merged 2 commits into from
Nov 22, 2017
Merged

[add] cms_notification #13

simahawk merged 2 commits into from
Nov 22, 2017

Conversation

@simahawk
Copy link

@simahawk simahawk commented Mar 24, 2017 •
edited
Loading

CMS form for managing personal notifications settings.

Depends on mail_digest PR and cms_form PR

@simahawk simahawk force-pushed the add-cms_notifications-PR branch from 3f3ab41 to ffc9e83 Compare April 21, 2017 09:23
@simahawk simahawk force-pushed the add-cms_notifications-PR branch from 7c418d1 to e20222b Compare May 15, 2017 11:57
guewen
guewen reviewed May 16, 2017
View reviewed changes
Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

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

Performance improvement suggestion.
I like the feature!

cms_notifications/models/res_users.py Outdated
('needaction_partner_ids', 'in', item.partner_id.id),
('subtype_id.cms_type', '=', True),
]
item.has_unread_notif = bool(msg_model.search_count(domain))
Copy link
Member

@guewen guewen May 16, 2017

Choose a reason for hiding this comment

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

With a large number of mail.message, a search with a limit=1 is drastically faster than a search_count. It has to search only 1 line vs all of them.

@simahawk simahawk force-pushed the add-cms_notifications-PR branch from f002c62 to 1a8157d Compare May 29, 2017 17:46
@pedrobaeza
Copy link
Member

pedrobaeza commented May 30, 2017

Please don't use plurals on module name. cms_notification is good enough I think, or if not, cms_multi_notification.

@simahawk
Copy link
Author

simahawk commented May 30, 2017

@pedrobaeza is it that fundamental? I have it already in production and I'd love to skip a painful migration here 😛

@pedrobaeza
Copy link
Member

pedrobaeza commented May 30, 2017 •
edited
Loading

Yeah, I think so. It's already a guideline and avoid typos. You can migrate it very easily with openupgradelib and shell command:

from openupgradelib import openupgrade

openupgrade.update_module_names([('cms_notifications', 'cms_notification')])

@simahawk
Copy link
Author

simahawk commented May 30, 2017

@pedrobaeza didn't knew that. TIL thanks :)
I'll rename and squash.

@pedrobaeza
Copy link
Member

pedrobaeza commented May 30, 2017 •
edited
Loading

EDIT: I have fixed the syntax of the code that was incorrect

@simahawk simahawk force-pushed the add-cms_notifications-PR branch from 1a8157d to a636a1a Compare May 30, 2017 13:20
@simahawk simahawk changed the title [add] cms_notifications [add] cms_notification May 30, 2017
@simahawk simahawk force-pushed the add-cms_notifications-PR branch from a636a1a to bd8161d Compare May 30, 2017 13:30
@simahawk
Copy link
Author

simahawk commented Jun 19, 2017

You can hit this button too after OCA/social/pull/158 is merged ❤️

@simahawk
Copy link
Author

simahawk commented Jun 21, 2017

@pedrobaeza OCA/social#158 merged, any blocker here?

pedrobaeza
pedrobaeza requested changes Jun 21, 2017
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Why don't you use FontAwesome included in Odoo?

cms_notification/README.rst Outdated
})
return res
Usage
Copy link
Member

@pedrobaeza pedrobaeza Jun 21, 2017

Choose a reason for hiding this comment

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

This is incorrectly indented

cms_notification/i18n/cms_notifications.pot Outdated
@@ -0,0 +1,81 @@
# Translation of Odoo Server.
Copy link
Member

@pedrobaeza pedrobaeza Jun 21, 2017

Choose a reason for hiding this comment

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

Remove POT file

oca_dependencies.txt Outdated
#
# To provide both the URL and a branch, use:
# sale-workflow https://github.com/OCA/sale-workflow branchname
social https://github.com/camptocamp/social add-mail_digest
Copy link
Member

@pedrobaeza pedrobaeza Jun 21, 2017

Choose a reason for hiding this comment

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

This should be changed

@simahawk
Copy link
Author

simahawk commented Jun 21, 2017

@pedrobaeza font-awesome from odoo is old, at least in v9

@pedrobaeza
Copy link
Member

pedrobaeza commented Jun 21, 2017

Yeah, but possibly the icons (or similar) used here are in this version, because bundling libraries on each module is very heavy, and more if the use is very light as this one.

@simahawk simahawk force-pushed the add-cms_notifications-PR branch from bd8161d to 6255bff Compare June 21, 2017 10:10
@simahawk
Copy link
Author

simahawk commented Jun 21, 2017

@pedrobaeza they are not. There's no envelope open for instance. This is why I added it. To be honest: I'd say that icons should be updated in OCB. Here, maybe, I can create a custom one...

@pedrobaeza
Copy link
Member

pedrobaeza commented Jun 21, 2017

OK, what about having a module base_fontawesome with this lib? On v10, we can deprecate it as the version is more modern. Can you obtain the same way of declaring icons with this new module, so you don't have to change anything in this module?

@simahawk simahawk force-pushed the add-cms_notifications-PR branch from 6255bff to 9311a7c Compare June 21, 2017 10:34
@simahawk
Copy link
Author

simahawk commented Jun 21, 2017

hmm could be a good idea and allow to reuse it for several v9 modules.

@simahawk simahawk mentioned this pull request Jun 21, 2017
Merged
@simahawk
Copy link
Author

simahawk commented Jun 21, 2017

@pedrobaeza there you go OCA/server-tools#866

@simahawk simahawk force-pushed the add-cms_notifications-PR branch from e3159cd to fecd4e4 Compare November 22, 2017 13:30
@simahawk
Copy link
Author

simahawk commented Nov 22, 2017

final bits to get this merged: replace oca dependency url as OCA/server-tools#866 has been merged a long time ago.

@simahawk simahawk merged commit 7b1e371 into OCA:9.0 Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guewen guewen guewen left review comments

@pedrobaeza pedrobaeza pedrobaeza requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@simahawk @pedrobaeza @guewen