Saurabh, I apologize for my confusion. I just realized that the patch you attached to issue 1590 contains revisions for issue 1587, not 1592. My first test does indicate that the revisions fix the defect. I will continue to review it. For the moment please ignore my suggestions below.
However, for the sake of future readers, please attach the patches to the correct issues. Thanks, and I am very sorry for the confusion. Keith On Tue, Feb 26, 2008 at 10:49 AM, Keith <[EMAIL PROTECTED]> wrote: > Hi, Saurabh, > > Thanks again for your submissions. I've reviewed your two patches and > have these comments: > > 1. The patches that you attached to the issues do not correspond to > the defects that the issues address: > > * The patch attached to issue 1587 > (https://mifos.dev.java.net/issues/show_bug.cgi?id=1587) > actually contains revisions to fix issue 1590. > > * The patch attached to issue 1590 > (https://mifos.dev.java.net/issues/show_bug.cgi?id=1590) > actually contains revisions to fix issue 1592. > > After considering the comments that follow, please attach the correct > patches to the correct issues. That will help facilitate our review > quite a bit. > > 2. The patch for issue 1590 (that is attached to issue 1587) does fix > that defect. However, I cannot commit it because the patch depends on > revisions submitted for issue 1592 (attached to issue 1590). > Specifically, you have included revisions to the class Constants.java > in the 1592 patch but did not include them in the 1590 patch. > > 3. The patch for issue 1592 (that is attached to issue 1590) does not > correct the defect. The drop-down box that should display acceptable > payment types for fee payment still incorrectly displays the > acceptable types for loan disbursement. > > I look forward to your resubmission of these patches. > > Also, did you plan to submit a patch for the closely related issue > 1587? > > Keith Pierce > > On Feb 10, 10:11 pm, "Aliya Walji" <[EMAIL PROTECTED]> > wrote: > > Hi Saurabh, > > > > Thanks so much for attaching the patches to the issue. Just a quick > > reminder to you and the rest of the community around the process, please > > make sure to also add the "Patch' keyword to the item in the issue > > tracker so that we can keep track of items in the issue tracker that are > > awaiting code review and check-in. I've added the keyword for the two > > items below. > > > > Thanks, > > > > Aliya > > > > ________________________________ > > > > From: [EMAIL PROTECTED] > > [mailto:[EMAIL PROTECTED] On Behalf Of > > Saurabh Kumar > > Sent: Thursday, February 07, 2008 3:35 AM > > To: Developer > > Subject: Re: [Mifos-developer] Patch for defects 1587,1590 and > > suggestion ......... > > > > Hi Aliya, > > > > I have attached the patches in the issue tracker. > > > > https://mifos.dev.java.net/issues/show_bug.cgi?id=1587 > > > > https://mifos.dev.java.net/issues/show_bug.cgi?id=1590 > > > > Thanks & Regards, > > > > P Think before you print > > > > Saurabh Kumar * Developer * SunGard * Technology Services * Divyasree > > Chambers, Langford Road, Bangalore 560025 India > > Tel+91-80-2222-0501* Mobile+91-9886945575* Fax +91-80-2222-0511 * > www.sungard.com<http://www.sungard.com/> > > > > -----Original Message----- > > From: [EMAIL PROTECTED] > > > > [mailto:[EMAIL PROTECTED] On Behalf Of > > Aliya Walji > > Sent: Monday, February 04, 2008 10:37 PM > > To: Developer > > Subject: Re: [Mifos-developer] Patch for defects 1587,1590 and > > suggestion ......... > > > > Hi Saurabh, > > > > Can you attach the patches to the respective bugs in the issue tracker? > > This is our new process for tracking patches that need to be reviewed. > > > > I'll let a developer comment on your suggestion below :-) > > > > Thanks, > > > > Aliya > > > > ________________________________ > > > > From: [EMAIL PROTECTED] > > [mailto:[EMAIL PROTECTED] On Behalf Of > > Saurabh Kumar > > Sent: Monday, February 04, 2008 2:21 AM > > To: [EMAIL PROTECTED] > > Subject: [Mifos-developer] Patch for defects 1587,1590 and suggestion > > ......... > > > > Hi, > > > > Please find the patch for defects 1587 and 1590. > > > > Please note that apart from bug fixing I have removed some hard coding > > of strings from code. I have added constants for these strings in > > Constants.java file. > > > > 1587 >> Define Accepted payment methods not working for clients/groups > > > > https://mifos.dev.java.net/issues/show_bug.cgi?id=1587 > > > > 1590 >> Define accepted payment methods not separated for savings > > deposits/withdrawals > > > > https://mifos.dev.java.net/issues/show_bug.cgi?id=1590 > > > > Suggestion >> > > > > I have noticed that at many places we are using hard coding for empty > > string and some common strings like "savings", "loan", etc. Also in > > getSecurity() method of all the action classes we are using hard coded > > values for strings like "load", "preview", "previous". We can move all > > these constants to Constants.java file or to the constants files of > > there respective modules. We can voluntarily take up this task while > > fixing bugs, i.e, while fixing bugs we can remove hard coding for common > > strings from the corresponding action classes. > > > > Thanks & Regards, > > > > P Think before you print > > > > Saurabh Kumar * Developer * SunGard * Technology Services * Divyasree > > Chambers, Langford Road, Bangalore 560025 India > > Tel+91-80-2222-0501* Mobile+91-9886945575* Fax +91-80-2222-0511 * > www.sungard.com<http://www.sungard.com/> > > > > > > > > > ------------------------------------------------------------------------- > > 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/ > > ------------------------------------------------------------------------- > 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/ >
------------------------------------------------------------------------- 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/
