Pramod, I tried to apply your latest reports patch in Eclipse to base revision 12252, but got many errors. Eclipse's patch review screen showed a total of 33 unmatched hunks spread over 15 files.
The preview window's comparison panes seemed to show a lot of extra blank lines in the patches, which was puzzling. These also showed up in TextPad -- every other line in the file is blank. A hex dump of the file revealed that each line was preceded by an extra CR character (that is, each line of the patch file is terminated by CR CR LF, instead of the usual CR LF. I suspect that these extra CRs are confusing Eclipse's patch application logic. Your previous patch submissions did not display this behavior -- each line terminated with the normal CR LF. Perhaps some text editor is adding extra CRs. Would you fix the patch by ensuring that lines are terminated correctly and resubmit? Keith Pierce On Jan 14, 2008 5:00 PM, Van Mittal-Henkle <[EMAIL PROTECTED]> wrote: > Forwarding this reply to the dev list after removing the attachment > (patch) which was too big for the list. > --Van > > ---------- Forwarded message ---------- > From: Pramod Biligiri <[EMAIL PROTECTED]> > Date: Jan 14, 2008 11:37 AM > Subject: Re: [Mifos-developer] Patch file related to Collection Sheet > Report > To: Developer <[email protected]> > Cc: Amiruddin Nagri <[EMAIL PROTECTED]>, > [EMAIL PROTECTED], Keith Pierce <[EMAIL PROTECTED]>, Nandini > Yadalam < [EMAIL PROTECTED]> > > > > Hi, > I've responded to most of the issues you raised in this mail. I've > also attached a patch with the changes as explained below. > (You don't need to use TortoiseSVN for this). A few test cases are > failing, > so please don't consider this patch as final. But you should be able to > review most of the code changes nevertheless. > > > * 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. > > I had to retain some of the private constructors because some fields are > final, and > have to be assigned in a constructor only. I have added methods called > createInstanceForTest in these cases. Where only non-final fields are > involved, the code now uses the no-arg constructor and an instance method > called populateInstanceForTest. > > > > > * in the class PersonnelLevelEntity, a static instance variable > > NON_LOAN_OFFICER_LEVEL_ENTITY has been added. It appears to be unused > > This has been removed now. > > > > > * 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. > > Could you indicate to where I can move this? I see classes called > Localization and ConfigLocale which are invoked by DateUtils.getLocaleDate > . > Similarly for the string constants in ReportsConstants.java, is there an > existing location or should I create a new file? > > > > > * 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. > > I have merged DateSelectionItemPersistence into SelectionItemPersistence > now. > > > * 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? > > - Correct. This is to make them pass through the > BirtReportValidationController > servlet which does the error handling. > > > > > * 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. > > > - Unless I am mistaken, these changes were not added in a commit by us. > Can you please confirm? > > > > > * 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)? > > - By having this as a separate servlet, it's easier to add validation for > new reports as more of them are added, based on a hashmap lookup. > Also, control flow is like this: When the Generate button for a report is > clicked, that iframe is submitted to this servlet which checks for errors. > Any errors are added as request attributes and passed on to Birt. After the > Birt Servlet returns, these attributes are picked up > in a JSP which renders the parameter window, and displayed in red text. > > > > > Note: Please use TortoiseSVN to apply this patch. > > > > I'm curious, why were you suggesting the use of TortoiseSVN? > Oh, that's what we have been using here. So I was recommending that for > best results. > > > > Pramod Biligiri, > App Dev - ThoughtWorks > > > ------------------------------------------------------------------------- > 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 >
------------------------------------------------------------------------- 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/
