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] <mailto:[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
