Hi Pramod, > Please find attached a patch file related to the Collection Sheet > Report. > This patch adds caching support, and some formatting improvements to > the report
Thanks for the patch! Here is some feedback based on the patch that was submitted: * first off, nice work! The basic design and code you have been working on looks good. Below you will find some more detailed feedback and questions. Please send an updated patch that addresses the issues outlined below. * many new business object constructors have been added in core classes such as LoanBO, SavingsBO, CustomerBO, CenterBO (and others). These constructors appear to be only used for testing (and in particular testing reports). There is risk that a developer could try to use them to construct real business objects, thinking that they are constructing business objects in a valid state. Is there another way that the testing could be accomplished? For example, what if an object were created with a no argument constructor and then populated with a populateTestInstance(<args>) method with the same arguments as the constructors that have been created? It seems that something along these lines would keep the code cleaner. * in the class PersonnelLevelEntity, a static instance variable NON_LOAN_OFFICER_LEVEL_ENTITY has been added. It appears to be unused and I am guessing that it was added for testing purposes. If an instance like this is need for testing, is should be added to a class in the test directory hierarchy rather than to the class itself. Please either remove this, or move it under the test hierarchy. * in the class CollectionSheetReportParameters, the static variable REPORT_DATE_PARAM_FORMAT will need to be generalized to support i18n (internationalization). There are various places in the code where this same kind of usage is present, but we are in the process of eliminating hard coded usage like this, so we want to avoid introducing new instances that will need i18n work. * in the package src/org/mifos/application/reports/persistence, the class DateSelectionItemPersistence includes only a single method. In current usage Persistence classes are intended to handle retrieval of groups of related objects or perhaps a single class if there were many methods related to manipulating that class. The usage here seems too fine grained. * in the class org.mifos.application.reports.util.helpers.ReportsConstants, there are constants such as SELECT_DISPLAY_NAME, which are hard coded for English. These should be references to resource bundle entries, so that they can support i18n. * in src/org/mifos/doc-root/application/reports/jsp/birtReport.jsp can you explain the change of the url from /run to /birtReports? It appears that this is to hook up to the changes made to the web.xml file, is that correct? * as mentioned in a previous email Upgrade167 is commented out in DatabaseVersionPersistence which is causing test failures. In general, we would like patches to be submitted only after the test suite is passing. If there are test failures which you can't figure out, then post to the list (as Amiruddin did) to ask for ideas. If you are sending a patch for which the test suite is not passing (ie. The run_test ant target fails) the please clearly indicate this when sending in the patch. That will help us to understand that the patch being sent is not ready to be committed, but is being sent to help debug or diagnose the problem. * in src/org/mifos/META-IN/struts-config.xml could you describe what changes in Mifos error handling behavior you expect from the global-exceptions entries that were added. * could you describe how you made the design decision to run the birt reports validation as a separate servlet (as defined in src/org/mifos/META-IN/web.xml)? * could you describe why you chose to load a Spring application context in org.mifos.application.reports.business.service.CollectionSheetReportServ iceFactory separately from the application context that is already defined in Mifos? Is this a short term solution or do you feel there is a reason for retaining this usage? * CollectionSheetTestSuite was added to the top level test suite ApplicationTestSuite. Although there is currently no documentation to indicate this (I need to add documentation to this effect), when you add a test to ApplicationTestSuite, it currently must also be added to an ApplicationTestSet (1,2,3 or 4). For the time being, please add it to ApplicationTestSet1. The test sets are used to run tests in parallel when multiple cores or processors are available to do so. > Note: Please use TortoiseSVN to apply this patch. I'm curious, why were you suggesting the use of TortoiseSVN? Cheers, --Van
<<winmail.dat>>
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
