Pramod, I've reviewed the corrections you've made in response to Van's comments. It looks like you've implemented nearly all of Van's suggestions, all that you were able to with the information that you had at the time. Here are some final comments, mainly to record future work that needs to be done before going live with the new reports.
1. Patch creation. As is pointed out in other submissions in this thread, we had problems merging the patch, apparently due to your use of SVK's patch-creation services. It doesn't really matter what process you use to create patches, as long as they can be applied cleanly. Please be sure to test your patch locally before submitting. 2. Use of no-argument constructors for creating test versions of business objects. These constructors are required by Hibernate, and should never be used to construct business objects outside of Hibernate. This practice is dangerous in that it bypasses business logic embedded in the full constructors, and thus likely leads to the creation of business objects. However, in order to get complete coverage of unit tests, it may be necessary to do so given how business objects are architected today. If necessary, please use the patterns you've put in place as suggested by Van -- adding methods like "createInstanceForTesting()" -- instead of using dangerous constructors directly. Specifically, PrdOfferingBO has a one-argument constructor that should not be public, which should be fixed in a future patch. 3. In the class CollectionSheetReportParameters, the static variable REPORT_DATE_PARAM_FORMAT will need to be generalized to support i18n (internationalization). The internationalization team (Van) will publish guidelines on where to put such parameters to support the new paradigm. When it does, this parameter will have to be moved in a future patch. 4. 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. Again, this will have to be changed in a future patch, once I18N guidelines have been published. 5. The design decision was made to run BIRT report validation in a separate front servlet. We need to discuss whether this is the approach to be used going forward. Keith Pierce On Jan 17, 3:26 am, Pramod Biligiri <[EMAIL PROTECTED]> wrote: > [EMAIL PROTECTED] wrote on 01/17/2008 01:32:43 > AM: > > > Pramod, is this patch against SVN revision 12252? I tried applying the > > patch to a fresh working copy updated to 12252 but there were too many > > errors to count. > > Hi Adam, > Van has committed this patch in the mifos code base in revision 12257. > Perhaps you can pick it up from there. I just updated to SVN revision > 12258 and am running all the test cases right now. > > Pramod Biligiri, > ThoughtWorks > > > > > On Jan 16, 2008 8:16 AM, Pramod Biligiri wrote, > > > Hi Keith, > > > I tried applying the patch again after reverting my code again. No > > > problems. The project builds too (I didn't run the tests this time). > > > > One thing I noticed is that for both these files (CenterBO.java and > > > GroupBO.java), the latest commit seems to have been today (Wednesday > > > 16 Jan by vanmh): > > > > Last commit revision: 12:32:37 am, Wed, Jan 16, 2008 - vanmh > > > > Perhaps that has something to do with it? Hope that helps... > > > > Pramod Biligiri, > > > ThoughtWorks > > > > ------------------------------------------------------------------------- > This SF.net email is sponsored by: Microsoft > Defy all challenges. Microsoft(R) Visual Studio > 2.http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
