Thanks - I am responding to the list... I still think we need to go with names and not numbers. Everything else looks OK now based on the description (I did not look at the code). D.
Saint, David (CAR:9D60) wrote: > Hi Damian, > > I've attached an updated version of the XX-5634 (E911) sipXconfig patch > based > on the changes you requested (main rev16362), my changes are noted > below. > >> user visible changes: >> >> It would be more user friendly if notification groups had names and >> not numbers. They can use 'id' internally (in configuration files) to >> ensure that changing the name won't change the way system notifies the > >> users. > > I tried changing this but ran into trouble with the default strings. > Currently the code using "0" for disabled and "1" for the default group > number, > but if we use actual "Disabled" and "Default" strings I'm not sure how > we will > handle different languages. > e.g. sipXsupervisor/etc/sipXalarms-config.xml > The must be a better way of doing that. If you need some indication of "default" or "disabled" it's better to make it explicit in the configuration file than to rely on specific values. (it's not a question of localization since it's only in config file, it's a question of someone adding the group with the name of "Default"). And with the exception of paging groups (where number is actually used for something) everything in sipXconfig has a name. >> There is something wrong with "edit" vs "add" functionality for the >> group. If I edit group number a new group is created. > > This should be working properly now. > >> Missing unit test: >> - TestIntegration needed for AlarmGroup add/edit/remove >> - TestUI needed for newly added screens >> - unit test for newly added alarm-groups file > > Added tests to AlarmContextImplTestIntegration.java, > AlarmsPageTestUi.java, > AlarmGroupConfigurationsTest.java > >> Other comments: >> - AlarmComponent class does not seem to be used anywhere - should be >> removed (or used?) > > Removed that class. > >> - What happens if user is removed - does Alarm Group that references >> such user gets modified? > > This works properly now, user gets removed from the group. > >> - It would probably make sense to have AlarmGroupManager separate from > >> AlarmContext. AlarmContext can be user to raise alarms. <anaging >> notifications is a separate functionality. >> (I realize it's been broken even before your changes but notification >> management got more complicated now). No problem. I will split it when taking the patch. It's been hijacked before you looked at this, so no reason why you should be fixing it. > > Since I used EditPagingGroupPage.java as the basis for > EditAlarmGroupPage.java > I'm a bit reluctant to make this code too different from the original, > but I can try if you feel strongly about that. > >> - AlarmGroupsPanel should be implemented in .java and .html file (no >> ,jwc file) - also it should be used in AlarmsPage: >> from what I am seeing it's been implemented but it's not referenced >> anywhere. > > This is fixed now, and referenced in AlarmsPage.java. > >> - It would be better if AlarmGroup were not persisted on >> EditAlarmGroup page: the ID should be persisted and group should be >> loaded from DB in pageBeginRender (it probably contributes to the >> observed add vs. edit confusion). > > Tried making AlarmGroup not persistent but that didn't work properly; > after adding a user and returning to the AlarmGroup menu the previously > entered data had disappeared. > This code is based on EditPagingGroupPage.java which persists > PagingGroup. OK. I thought that there is a better way of doing that. At the minimum we need to make sure it's cleaned properly when the page is used for another group. I'll have a look. > >> - UserExceptions don't have to be caught - they are handled by >> automatically and should result in errors visible by the user > > The reason for catching this exception is so that we can put up the > custom string indicating the alarm group number (same as for the > paging group number in EditPagingGroupPage.java) > It's probably the old code. All UserExceptions are handled automatically and the handler would do exactly what's done here. I'll remove the specific handler and see if the error message is still displayed correctly. > Regards... > Dave. > > _______________________________________________ sipx-dev mailing list [email protected] List Archive: http://list.sipfoundry.org/archive/sipx-dev Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev sipXecs IP PBX -- http://www.sipfoundry.org/
