[
https://jira.nuxeo.org/browse/NXP-4950?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=72075#action_72075
]
Thierry Delprat commented on NXP-4950:
--------------------------------------
Hi, thanks for this great contribution.
We will take a deeper look as soon as possible.
But after a first look :
- the feature looks good
- the packaging and separation between api, implementation, config ... is very
good
- you have unit tests
- the doc and code looks clear
=> That's a really great work, thx !!!
Here are some quick remarks / questions :
* notifications-core :
Could we have a diff for the patch on notification core ?
By the way, I completly agree that this service needs cleanup and refactoring.
Provided the diff, we can try to make it a little bit better.
* ecm:lock :
I suppose your code depends on a patch on the QueryMaker to make the locks
searchable ?
* UnrestrictedSession
Be careful about usage of repository sessions.
Typically, in DeferredAlertManagementServiceImpl.getAdminSession you create an
Admin CoreSession :
- that session is not released
=> leak
- the JAAS Login context is not cleaned
=> expect to have problems :)
- the session can be accessed by several threads (because
DeferredAlertManagementServiceImpl is a singleton)
=> you will have issue in a JTA environment
A simple solution would be to use the UnrestrictedSessionRunner to manage your
admin session.
Do not bother about caching the CoreSession : the JCA Pool will do it for you.
=> This will make the code simpler and safer
* Packaging
DeferredAlertManagementServiceImplComponent should be in the core package I
think ?
* TasksProvider
You use direct Hibernate query.
Why not using the getCurrentTaskInstances of the JBpmService ? (optimization ?)
* Error Management
It's always better to do explicit logging of errors or rethrowing the error
rather that a simple e.printStackTrace() (I know that this is eclipse's fault
:) ).
* DocumentModelAdapter
You have several Wrappers (like AlertSettingsWrapper) that look to me like a
DocumentModelAdapter.
You may want to use this model rather than a manual wrapper :
- it's more "standard"
- adapter can be cached inside the DocumentModel (usefull inside JSF/Seam)
Tiry
> User settings management and Batch notifications
> ------------------------------------------------
>
> Key: NXP-4950
> URL: https://jira.nuxeo.org/browse/NXP-4950
> Project: Nuxeo Enterprise Platform
> Issue Type: New Feature
> Affects Versions: 5.3 GA
> Environment: Tested on ubuntu + tomcat + derby, ubuntu + jboss +
> postgresql
> Reporter: Christophe Capon
> Attachments: AlertManagement.tar.gz
>
>
> Our nuxeo user start complaining about Nuxeo verbosity regarding
> notifications. A sustained activity on the database may trigger a significant
> amount of mails.
> To resolve this point, we have developed two services :
> 1) An alert manager service : it collects all the alerts worth being notified
> to an user (locked documents, overdue tasks, subscriptions...). It groups all
> those alerts in a single mail and sends it periodically to the user. This
> service can be extended via extension points, if you want to add other alerts
> (eg a document has reached its expiration date,....). At this point, we need
> to allow the user to customize some parts of the alert mail.
> 2) There comes the second service. We want to allow the user to setup the
> frequency of the alert mail, to select what he wants inside the mail (in our
> development, locked documents and overdue tasks are sent no matter what the
> user wants, but the user may want to have all his future tasks, or tasks
> without target date). We created a UserSettings management service, allowing
> the user to select his preferences. This service is similar to the Windows
> Control Panel or Ubuntu Preferences Menu. So far, it only handles the alert
> configuration, but you can add other settings managers through extension
> points.
> A first stable but perfectible release is almost done, and I will keep you
> posted when the code is available, with unit tests and hopefully selenium.
> Hopefully, this is the right place to post my code ?
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
https://jira.nuxeo.org/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
_______________________________________________
ECM-tickets mailing list
[email protected]
http://lists.nuxeo.com/mailman/listinfo/ecm-tickets
