Hi Marc,
Your contribution is integrated in branch V7.0.
Thanks for your efforts to bring a new feature and set it at ProjeQtOr quality level.
Here are new remarks, with some improvements / fixing to finalize this feature
POINT 3
On projeqtor_v7.0.sql
Rather that
INSERT INTO XXX (A, B)
VALUES (1,2);
INSERT INTO XXX (A, B)
VALUES (2,3);
group in one single quey
INSERT INTO XXX (A, B) VALUES
(1,2),
(2,3);
=> really minor remark, to take into account for future contributions
POINT 4
Removing screen Notification Type is OK,
but you also removed creation of the 3 types.
=> Fixed in projeqtor_v7.0.sql
POINT 7
Renaming NotificationObjectClass to Notifiable is good.
But Notifiable list should be initialized.
=> Done, inserted : Action, Activity, Bill, Command, Deliverable, Delivery, Incoming, Issue, Meeting, Milestone, Opportunity, ProjectExpense, Quotation, Requirement, Risk, Ticket, Term. Possibly, this list could be extended (let's start simple, and extend it on community request).
POINT 10
Thanks for the explanation.
I understand better now.
So we'll have :
- indicator : will highlight item on expected value (date, work, cost) and possibly alert only once "before target date".
- notification : will notify on expected date, and possibly before given days, and possible repeat notification on monthly / yearly basis
=> OK, see point 21 for expected improvement (remind every day / week)
POINT 16
OK, it appears now.
See point 24 for small issue discovered.
=> We will remove bottom bar on V7.0.0. It is a major design change.
We'll add a top bar, including project selector, navigation buttons (including new tab) and new "User Profile" access.
The top bar menu, will become optional (hidebale).
We'll move your notification on the new top bar, beside the "User profile" icon.
Pay attention for further patches to always take into account latest revision, and send only "difference patch" for your new changes in order not to erase our changes.
POINT 17
Great, I like the way you did it.
Very logical, even if not standard.
=> If user have "write" access, he can create manual notifications, if he has "readonly" access, he cannot create new manual, but can change his own (to mark them as read)
Well done !
POINT 19
Great, new interface is much more explicit, and help system is great !
=> I saw some entries in lang.xls for "menuNotificationDefinitionNovice" and "NotificationDefinitionNovice" but this class and menu do not exist.
I removed them, guessing it was some first idea of yours to have advanced and novice interface, but finally proposed only one form for both user types.
Can you confirm ?
=> I also saw that on js file, you use condition "if(context==='Novice')".
But it seems that this comes from parameter of function called only in NotificationDefinition::getValidationScript() where parameter is always set to "".
So condition may never be true...
Can you please "clean" this part of code ?
=> I made a small reorganization of sections, to get a better use of screen space.
=> I made some little "cosmetic" changes to increase understanding of use
POINT 20 (NEW)
New items in lang.xls redefine some caption already existing.
Better use existing translation code rather than creating duplicate.
This will avoid extra large lang files and will ease translation for community translators.
or => OR
and => AND
buttonInsertInTextBoxNotificationDefinition => operationInsert
colIdNotificationStatus => [Not used]
colIdStatusNotification => colIdStatus
idNotificationType => idType
=> really minor remark, to take into account for future contributions
POINT 21 (NEW)
Notification System can remind every month / year
=> I think it would be interesting to add possibility to notify every "day" (or open day only) and "week"
For instance for an action not done, I could notify responsible every (open) day or every week (depending on urgency of action)
POINT 22 (NEW)
Notification System can remind "before" given days.
=> I thing for some items or some dates, it would be good to notify before given "hours" (or possibly minutes, but it would require higher notification frequency than 3600s)
For instance, for a Ticket, I would like to notify responsible 1 hour before ticket due date/time.
Or for meeting, I could notify 15 mn before meeting date and time (more complex as meeting has 2 fields for date and time)
POINT 23 (NEW)
Screen "Notification Definition" and "Notification" don't have a user manual entry.
I think it is important to explain how the system works (need to update some .rst files in docs/user)
=> Please create entry in user doc
POINT 24 (NEW)
When defining TITLE and CONTENT in notification definition, fields are inserted as #{field}.
Similar system already exists (for instance for email titles, and for new Email Template system) with syntax ${field}
=> Please upgrade system to use existing syntax with ${}
POINT 25 (NEW)
On the notification icon (on bottom bar), number on "unread" notifications is wrong. It seems "manual" notifications are not taken into account.
(I had 3 notifications, 1 manual, 2 automatic, icons show "2" notifications only).
It seems the same for the "unread notification" on message section (above console message). As long as I have only one manual notification, it is correctly displayed.
If I also have automatic notification, unread manual notification is not displayed.
=> Please fix this issue (if confirmed).
POINT 26 (NEW)
Help to insert field in content (using CK-Editor) does not work : nothing happens
Error logged in browser console : Cannot read property 'getValue' of undefined at addFieldInTextBoxForNotificationItem (projeqtor.js:4468)
=> Please fix this issue.
POINT 27 (NEW)
Notification and NotificationDefinition are not supposed to be changed through "Screen Customization" plugin.
So structure with Class and ClassMain should not be used.
=> I removed Notification and renamed NotificationMain to Notification
POINT 28 (NEW)
There was an error on api/api.php and too/adminFunctionalities.php : closing } was included in comment, so these scripts were not working anymore
=> Fixed
POINT 29 (NEW)
Icons for standard themes (non flat) where not colorfull as expected.
Moreover, it's not necessary anymore to design each icon in three sizes (use css3 background-size)
=> Fixed
POINT 30 (NEW)
I don't understand use and interest of _drawLike_xxxx, that has high impact on objectDetail.php
=> Please explain