[Mifos-developer] Patch file related to Collection Sheet Report
Pramod Biligiri
Hi,
Please find attached a patch file related to the Collection Sheet Report.
This patch adds caching support, and some formatting improvements to the report
Note: Please use TortoiseSVN to apply this patch.
Pramod Biligiri,
App Dev, ThoughtWorks
Van Mittal-Henkle
> 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
Pramod Biligiri
Hi,
Regarding the Spring application context issue raised below: There's no specific
reason to load the reports related beans in a separate context, from a different
XML file. It was done because we could not find a straightforward way to access
the existing instance of the Spring context from within our code. reportServices.xml
can continue to be retained as a separate XML file because it's more manageable.
Pramod Biligiri,
ThoughtWorks
mifos-devel...@lists.sourceforge.net wrote on 01/14/2008 12:15:05 PM:
> Hi Pramod,
Adam Monsen
sheet patches. These messages were sent off-list.
Hopefully having the conversation on-list will allow us to work
together to figure out what went wrong with the patch process and help
us make things work more smoothly in the future!
Pramod introduced me to rapidshare (thanks, Pramod!) so I'll use that to
provide a link to share the patch. File hosting services are handy since
they avoid the mailing list restrictions on attachments, and I think
linking to patches instead of attaching them is generally fine since
patches have a very limited lifetime.
http://rapidshare.com/files/84328744/16Jan-diff-2.patch.gz
When using an third-party file sharing service, it is also a good idea
to provide a secure hash of your file. That way others will know that
the content was not tampered with.
The SHA-1 checksum for the file "16Jan-diff-2.patch.gz" is
ffc50f8bffd2905ff04ba1f2bb4b946fc5b03e64
(The command-line program "sha1sum" was used to generate it).
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.
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
"Keith Pierce" <krp...@gmail.com> wrote on 01/16/2008 09:04:46 PM:
> > Hi, Pramod,
> >
> > As I apply your patch, I've observed some strange behavior in the
> > patch. Do you observe similar behavior?
> >
> > I followed your instructions to ignore 1 leading path segment, but
> > still saw conflicts in two files (unmatched segments),
> > src/org/mifos/application/customer/center/business/CenterBO.java and
> > src/org/mifos/application/customer/group/business/GroupBO.java, even
> > after ignoring white space. In the compare panes, it appeared that
> > Eclipse's apply-patch logic could not determine where to put the
> > insertions. Do you have any idea why I'm getting this behavior? I
> > looked closely at the diffs and they appear to be ok and in proper
> > unified diff format.
> >
> > I went ahead and applied the patch, generating .rej files, and then
> > manually applied those diffs. I am now running all unit tests
> > against the patch, and will send you the results (it takes about an
> > hour on my desktop). Hopefully the tests will complete before we
> > talk at 10 IST.
> >
> > Keith
On Jan 16, 2008 4:24 AM, Pramod Biligiri wrote:
> > >
> > > Hi all,
> > >
> > > Please find attached the patch for the Collection Sheet Report
> > > changes. This time I've created it using the command line diff
> > > tool as Adam Monsen suggested on the Mifos Developers list. I have
> > > tested applying it in Eclipse against revision 12254 of mifos svn.
> > > It passes all the test cases.
> > >
> > > When you use Eclipse's "Team->Apply Patch", make sure to look for
> > > the option "Ignore leading path name segments" and Select "1" from
> > > the drop down. And check the checkbox next to each new file for it
> > > to be added.
> > >
> > > I've also uploaded the corresponding war file to RapidShare:
> > > http://rapidshare.com/files/84229337/mifos.war
> > >
> > > Pramod Biligiri,
> > > ThoughtWorks
--
Adam Monsen
-------------------------------------------------------------------------
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/
Pramod Biligiri
mifos-devel...@lists.sourceforge.net 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.
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
Keith
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 <pbil...@thoughtworks.com> wrote:
> mifos-developer-boun...@lists.sourceforge.net wrote on 01/17/2008 01:32:43
> -------------------------------------------------------------------------
> 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/
Pramod Biligiri
Hi Keith,
I've changed the constructor in PrdOfferingBO from public to protected, which
you had raised below.
Please find the patch attached. It also contains minor formatting changes to
LayoutReport.rptdesign which I had spoken to you about. I have tested the patch
on mifos revision 12258 and run the testcases. (As before, ignore 1 leading path
segment when applying).
Pramod Biligiri,
ThoughtWorks