Re: [Mifos-developer] Next steps with the GK client family details feature
Chand...@sungard.com
This is to update everyone on the status of this feature.
Currently all the concerns raised regarding the code have been addressed
to. The acceptance tests were committed some time back.
We have cleaned up the jsps and upgraded the datasets to correspond to
our tests. We have also finished writing validations and junits for
these.
Also we have removed all javascript from the jsps and everything is done
on the server now.
All the tests are passing on the Hudson build.
For the next steps, it would be great if someone can review the latest
war/code and send feedback.
Regards
Chandan
-----Original Message-----
From: Adam Feuer [mailto:afe...@grameenfoundation.org]
Sent: Saturday, November 07, 2009 11:40 PM
To: Rao, Chandan; Kollam, Rajesh
Cc: Pandiyarajan, Deepak; Thandavarayan, Parthasarathy; Matt Duncan
Subject: Re: Next steps with the GK client family details feature
On 11/7/09 9:47 , "Chand...@sungard.com" <Chand...@sungard.com>
wrote:
> Regarding the failures on the Hudson, there were 5 on the last build,
I
> fixed one in the morning and I am looking into the other failures ( 3
> integration and 1 acceptance)
>
> Also as I informed you in the morning, we are still blocked on the
> dataset front. It would be great if Udai can help us out on Monday.
Chandan,
Are these failures because of the dataset issue?
-adam
--
Adam Feuer
afeuer at grameenfoundation dot org
http://mifos.org
http://www.grameenfoundation.org
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
Van Mittal-Henkle
Great progress on the client family details feature.
We've reviewed most of the current work and wanted to pass along the
feedback we have so far. Please go ahead and make these changes as soon
as you can and we will continue to provide feedback so we can wrap up
the work by the end of this week.
Specific items...
1. only "Spouse" and "Father" show up in the "Relationship" dropdown
on the family details page
2. ClientBO constructor code should not be duplicated
3. ClientBO.isKeyExists should be private
4. AddFamilyDetailsTest - multiple tests should not cover the same
functionality. For instance, goToMfiPageTest() covers exactly what
isFamilyInfoPageTest() does, then performs an additional step.
isFamilyInfoPageTest() is therefore unnecessary.
5. AddFamilyDetailsTest - if possible, avoid loading dataset many
times. Combine tests wherever possible while preserving readability.
6. AddFamilyDetailsTest - editAfterSubmit() - don't use
selenium.isTextPresent(). Instead, enclose the element in the JSP with
an id tag and use a selenium selector/locator for that id.
7. AddFamilyDetailsTest - test names must reflect exactly what is
being tested, ie: why the test exists. For instance, should
submitAfterPreviewPageTest() be named something like
canReachSubmitPage() ?
8. AddFamilyDetailsTest - verifyBadDate(), verifyNoFirstName(),
verifyNoLastName(), etc. appear to just test the mandatory field
mechanism. This is unnecessary, we have a standard means for validating
mandatory fields. Also, when checking failing conditions, consider using
the same "negative test" approach in DefaultAdminUserCanLoginTest, ie:
look for error messages rather than failure to progress to the next page
(also notice descriptive strings in assertions explaining assertion
failures).
9. AddFamilyDetailsTest - clean up by calling
propertiesHelper.setAreFamilyDetailsRequired(false) at the end of every
test since other tests (like AttachSurveyListTest) expect this to be
false.
10. In AttachSurveyListTest don't call
setAreFamilyDetailsRequired(false). This should be false by default.
If the value is true when this test runs then it must be coming from
another test that has not cleaned up state, so it should be reset in the
other test instead.
11. in ClientBO.deleteFamilyAndNameDetails (maybe others) use
getCustomerPersistence() rather than new CustomerPersistence()
12. In Upgrade223 remove unused methods (only the upgrade method
appears neccessary).
13. ClientRules - rename intializeFamilyCOnfig() to
intializeFamilyConfig()
14. ClientRules - familyDetailsRequired and
maximumNumberOfFamilyMembers should be private
15. make sure subversion manages newlines for all new text files (see
http://www.mifos.org/developers/wiki/DeveloperSetup#svn-line-endings ).
For files you've already added and commited, you must manually set the
svn:eol-style property to 'native'.
16. need more information in svn commit log messages (see
http://mifos.org/developers/wiki/CommitLogGuide ) try to surface the
"why"
17. all code should be formatted using the Eclipse Java formatter (our
settings are committed to source control and should be automatically
loaded)
18. delete application/src/main/sql/latest-schema_last.sql
19. delete application/Mifos.log.2009-11-07
20. don't add new constructors to ClientBO. The "many-constructor"
antipattern should be avoided. This should eventually be refactored to,
for instance, use the factory pattern.
21. in ClientCustAction remove commented out code
22. why does setValuesForFamilyEditInActionForm have "removing the
extra family member"?
Thanks,
--Van
> -----Original Message-----
> From: Chand...@sungard.com [mailto:Chand...@sungard.com]
> Sent: Monday, November 09, 2009 9:37 AM
> To: Adam Feuer; Rajesh...@sungard.com; mifos-
> deve...@lists.sourceforge.net
> Cc: Matt Duncan; Parthasara...@sungard.com;
> Deepak.Pa...@sungard.com
> Subject: Re: [Mifos-developer] Next steps with the GK client family
> detailsfeature
>
> Hi All,
>
> This is to update everyone on the status of this feature.
>
> Currently all the concerns raised regarding the code have been
> addressed
> to. The acceptance tests were committed some time back.
>
> We have cleaned up the jsps and upgraded the datasets to correspond to
> our tests. We have also finished writing validations and junits for
> these.
>
> Also we have removed all javascript from the jsps and everything is
> done
> on the server now.
>
>
> All the tests are passing on the Hudson build.
>
> For the next steps, it would be great if someone can review the latest
> war/code and send feedback.
>
> Regards
> Chandan
Adam Feuer
<va...@grameenfoundation.org> wrote:
> We've reviewed most of the current work and wanted to pass along the
> feedback we have so far. Please go ahead and make these changes as soon
> as you can and we will continue to provide feedback so we can wrap up
> the work by the end of this week.
Chandan and Rajesh,
I just wanted to clarify - we will accept this feature if you address
these issues.
regards
adam
--
Adam Feuer <adamf at pobox dot com>
Chand...@sungard.com
Thank you for the feedback.I will send an update on all these by end of
today.
Regards
Chandan
Chand...@sungard.com
> 1. only "Spouse" and "Father" show up in the "Relationship" dropdown
> on the family details page.
This issue is fixed by adding database upgrades for updating the spouse
father lookup with more values
> 9. AddFamilyDetailsTest - clean up by calling
> propertiesHelper.setAreFamilyDetailsRequired(false) at the end of
every
> test since other tests (like AttachSurveyListTest) expect this to be
> false.
This issue still exists. I tried to fix it by setting
propertiesHelper.setAreFamilyDetailsRequired(false) in AfterClass and
AfterMethod. But both didn't seem help. I tried setting this property to
false at the end of each method but that also didn't help. Also
surprisingly the test that was failing was AddNoteTest which wasn't
failing before.
For now I have added set
propertiesHelper.setAreFamilyDetailsRequired(false) in the setup for
this, so it clears
> 2. ClientBO constructor code should not be duplicated
> 20. don't add new constructors to ClientBO. The "many-constructor"
> antipattern should be avoided. This should eventually be refactored
to,
> for instance, use the factory pattern.
We have refactored the save process with family. Now we use a setter to
add details into clientFamilyDetailEntity set ( we pass null for
spousename in the old constructor). I hope this is the same thing that
you were trying to get at.
Other issues were pretty straightforward and we have resolved all of
them.
Regards
Chandan
-----Original Message-----
From: Van Mittal-Henkle [mailto:va...@grameenfoundation.org]
Sent: Tuesday, November 10, 2009 8:53 AM
To: Mifos software development
Cc: Thandavarayan, Parthasarathy; Pandiyarajan, Deepak; Kollam, Rajesh;
Rao, Chandan; Adam Feuer
Chand...@sungard.com
We just did a demo to GK and they were pretty happy with this feature's functioning. To put it in their words "it is like we imagined it would be".
But there were a few more requests to this features functioning.
1) They want an additional comments field in the family details page, this will be for each row.
2) Currently this feature (Apart from the changes in the names) doesn't do a log in the change log. Basically they would be more happy with a similar audit trail. Any pointers with regard to populating the change log for this table (Like I said it already does it for names) will be really great.
This is with regard to the date validation patch,
They want combos for dd:mm:yyyy in the client details page.
This was their feedback. We will get to working on this when everything till now is found okay .
I have committed all the changes made with regard to Van's feedback.
Regards
Chandan
-----Original Message-----
From: adam...@gmail.com [mailto:adam...@gmail.com] On Behalf Of Adam Feuer
Sent: Tuesday, November 10, 2009 9:31 AM
To: Mifos software development
Cc: Kollam, Rajesh; Adam Feuer; Thandavarayan, Parthasarathy; Pandiyarajan, Deepak; Rao, Chandan
Subject: Re: [Mifos-developer] Next steps with the GK client family detailsfeature
Emily Tucker
> 1) They want an additional comments field in the family details page,
> this will be for each row.
Did GK explain how they would use the additional comments field? Ie,
what type of info would be captured here?
Van Mittal-Henkle
Here's more feedback on the family details work:
> > 1. only "Spouse" and "Father" show up in the "Relationship" dropdown
> > on the family details page.
>
> This issue is fixed by adding database upgrades for updating the
spouse
> father lookup with more values
Since there is currently no way for a user to dynamically create a entry
in SPOUSE_FATHER_LOOKUP, you should be able to just pass a fixed id into
the method insertIntoSpouseFatherLookup. This means that no method
largestSpouseFatherId is needed.
A related question, I don't understand the comment about "client name
type is hardcoded to 3" and why the id of 3 is skipped in
latest-data.sql for SPOUSE_FATHER_LOOKUP. Could you clarify this?
> > 9. AddFamilyDetailsTest - clean up by calling
> > propertiesHelper.setAreFamilyDetailsRequired(false) at the end of
> every
> > test since other tests (like AttachSurveyListTest) expect this to be
> > false.
>
> This issue still exists. I tried to fix it by setting
> propertiesHelper.setAreFamilyDetailsRequired(false) in AfterClass and
> AfterMethod. But both didn't seem help. I tried setting this property
> to
> false at the end of each method but that also didn't help. Also
> surprisingly the test that was failing was AddNoteTest which wasn't
> failing before.
>
> For now I have added set
> propertiesHelper.setAreFamilyDetailsRequired(false) in the setup for
> this, so it clears
So something is still not right here. We don't want to have
dependencies between tests, so you need to figure out why AddNoteTest
fails and find a way to make the tests using setAreFamilyDetailsRequired
self contained. What you tried with AfterClass and AfterMethod sound
like reasonable approaches. I don't know why those would not work, so
you'll need to do more investigation on that.
> > 2. ClientBO constructor code should not be duplicated
> > 20. don't add new constructors to ClientBO. The "many-constructor"
> > antipattern should be avoided. This should eventually be refactored
> to,
> > for instance, use the factory pattern.
>
> We have refactored the save process with family. Now we use a setter
to
> add details into clientFamilyDetailEntity set ( we pass null for
> spousename in the old constructor). I hope this is the same thing that
> you were trying to get at.
This seems like a reasonable approach.
> Other issues were pretty straightforward and we have resolved all of
> them.
Nice work taking care of the other issues.
One additional small item:
23. In setValuesForFamilyEditInActionForm please rename the variable "i"
to something descriptive which indicates what is being counted
(familyMemberCount?)
And one larger item:
24. When starting from a database that has family member data in the
"old" format and then upgrading to database version 223 with the new
family member feature turned on, there are errors as described in issue
2567. (https://mifos.dev.java.net/issues/show_bug.cgi?id=2567) For this
feature to be used, data will need to be migrated from the old table to
the new. This should be done external to Mifos (in a SQL script, for
example) for the time being.
--Van
Chand...@sungard.com
"24. When starting from a database that has family member data in the
"old" format and then upgrading to database version 223 with the new
family member feature turned on, there are errors as described in issue
2567. (https://mifos.dev.java.net/issues/show_bug.cgi?id=2567) For this
feature to be used, data will need to be migrated from the old table to
the new. This should be done external to Mifos (in a SQL script, for
example) for the time being."
I have fixed this. The user will not be getting any exception now and
will be able to get the normal flow. I am trying to see if I can do more
here ( Like improve the code to provide a way to edit/add without
needing to migrate data for family details).
Will have an update for this in the evening.
"Since there is currently no way for a user to dynamically create a
entry
in SPOUSE_FATHER_LOOKUP, you should be able to just pass a fixed id into
the method insertIntoSpouseFatherLookup. This means that no method
largestSpouseFatherId is needed.
A related question, I don't understand the comment about "client name
type is hardcoded to 3" and why the id of 3 is skipped in
latest-data.sql for SPOUSE_FATHER_LOOKUP. Could you clarify this?"
In the create_clientPersonalInfo.jsp
In line 357, you should find this <html-el:hidden
property="clientName.nameType" value="3" />
And
In Java
ClientConstants.CLIENT_NAME_TYPE
Which will lead to NameType.CLIENT.getValue() which has value 3
hardcoded into it.
Nametype is relationship type
So I didn't want to cause problems in the existing code, so I skipped
the value 3
" So something is still not right here. We don't want to have
dependencies between tests, so you need to figure out why AddNoteTest
fails and find a way to make the tests using setAreFamilyDetailsRequired
self contained. What you tried with AfterClass and AfterMethod sound
like reasonable approaches. I don't know why those would not work, so
you'll need to do more investigation on that."
I will try to see if we can find something.
Regards
Chandan
-----Original Message-----
From: Van Mittal-Henkle [mailto:va...@grameenfoundation.org]
Sent: Wednesday, November 11, 2009 11:11 AM
To: Rao, Chandan; mifos-d...@lists.sourceforge.net
Cc: Thandavarayan, Parthasarathy; Pandiyarajan, Deepak; Kollam, Rajesh;
Adam Feuer
Subject: RE: [Mifos-developer] Next steps with the GK client family
detailsfeature
Chandan Rao
Initially the idea was to capture the educational details, but they
had a discussion within the team and decided that it would be better
to make it into a generic text field.
regards
Chandan
On Nov 11, 12:22 am, "Emily Tucker" <ETuc...@grameenfoundation.org>
wrote:
Ryan Whitney
Currently there is already a generic notes field available for clients.
Have you discussed with GK if this wouldn't work for them? What data do
they want to see in this note field?
Ryan
--
Ryan Whitney
Mifos Technical Program Manager
rwhi...@grameenfoundation.org
Mifos - Technology that Empowers Microfinance (www.mifos.org)
Chandan Rao
I am not sure about the usage of this field. They want this field per
member in the client Details page. It has to be dynamic.
regards
Chandan
On Nov 11, 4:09 pm, Ryan Whitney <rwhit...@grameenfoundation.org>
wrote:
> Chandan,
>
> Currently there is already a generic notes field available for clients.
> Have you discussed with GK if this wouldn't work for them? What data do
> they want to see in this note field?
>
> Ryan
>
> rwhit...@grameenfoundation.org
> Mifos - Technology that Empowers Microfinance (www.mifos.org)
>
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
> trial. Simplify your report design, integration and deployment - and focus on
> what you do best, core application coding. Discover what's new with
> Crystal Reports now. http://p.sf.net/sfu/bobj-july
------------------------------------------------------------------------------
Parthasara...@sungard.com
Thanks and Regards,
Parthasarathy T * Technical Lead * SunGard * Technology Services * Embassy Icon, #3, 6th Floor, Infantry Road, Bangalore - 560 001, Karnataka, India * Tel +91 80 3091 3183 * Mobile +91 99450 00394 * Main +91 80 3091 3000 * Fax +91 80 2222 0511 * www.sungard.com/sts
P Think before you print
CONFIDENTIALITY: This e-mail (including any attachments) may contain confidential, proprietary and privileged information, and unauthorized disclosure or use is prohibited. If you receive this e-mail in error, please notify the sender and delete this e-mail from your system.
Hi Ryan
regards
Chandan
> > ---------- Let Crystal Reports handle the reporting - Free Crystal
> > Reports 2008 30-Day trial. Simplify your report design, integration
> > and deployment - and focus on what you do best, core application
> > coding. Discover what's new with Crystal Reports now.
> > http://p.sf.net/sfu/bobj-july
>
> --
> Ryan Whitney
> Mifos Technical Program Manager
> rwhit...@grameenfoundation.org
> Mifos - Technology that Empowers Microfinance (www.mifos.org)
>
> ----------------------------------------------------------------------
> -------- Let Crystal Reports handle the reporting - Free Crystal
Chand...@sungard.com
An update,
Issue 2567 (https://mifos.dev.java.net/issues/show_bug.cgi?id=2567) is
fixed but migration of name_ids from the customer_name_detail table is
required to use the functionality properly.
I haven't really investigated why addNotes test was getting the
areFamilyDetailsRequired was getting set true for this test, but fixing
the issue 2567 fixed this issue.
There were two new issues that we noticed and fixed today, one was an
error had crept into the update family info feature when we refactored
it . It was deleting the client Name as we were not checking for the
name type being of type client. We fixed this.
There was another issue we noticed. In the family details page, after a
delete of a row if the user does refresh or presses back and does
refresh then the page would explode. This is just due to resubmission of
a old form. We handle this case in the actionForm by not letting it
attempt an initialization of the arrayList if the index exceeds the size
of the arrayList.
That's about it for now.
Van Mittal-Henkle
Thanks for your latest updates.
Adam M. and I went through some of the updated code and the team did
some additional testing today on these features and others which
generated additional issues to address. Here is our latest feedback.
follow-ups:
24. please mark issue 2567 fixed.
new feedback:
25. LatestTestAfterCheckpointIntegrationTest fails when run alone. This
test must pass on its own.
26. Issue 2570 (Upgrade223 is broken). Please try out
http://pastey.net/128265 and commit this change to branches/sungard if
it works for you.
27. Here's a dump of the database used by our latest trunk build deploy:
http://birch.mifos.org/trunkdb-2009-11-11.sql.gz . It must be possible
to upgrade this dataset by deploying a war built from branches/sungard.
28. There are 7 issues regarding this feature in issue tracker (
http://tinyurl.com/ye5qwas ) that are assigned to you (including those
mentioned above). Please go ahead and work on addressing these issues.
If you're having trouble reaching java.net, let us know your IP
address(es) and we'll make sure they're not being blocked. Also, let us
know if you can ping java.net, if you are getting server errors, or what
(if anything) is not working.
--Van
----
Van Mittal-Henkle
Mifos Software Developer
Grameen Foundation
va...@grameenfoundation.org
Chand...@sungard.com
All the follow up issues have been resolved, I have also marked issue
2567 as fixed.
Fixed broken upgrade, upgrade223
https://mifos.dev.java.net/issues/show_bug.cgi?id=2570
Fixed Date Format error in Date Field
https://mifos.dev.java.net/issues/show_bug.cgi?id=2572
Fixed Bad error message when validating family Details
https://mifos.dev.java.net/issues/show_bug.cgi?id=2573
Fixed the order in which hibernate retrieves client family details.
https://mifos.dev.java.net/issues/show_bug.cgi?id=2578
Fixed the delete link to work in IE.
https://mifos.dev.java.net/issues/show_bug.cgi?id=2571
Fixed bad display of date fields on browser resize.
https://mifos.dev.java.net/issues/show_bug.cgi?id=2577
Thank you and regards
Chandan
-----Original Message-----
From: Van Mittal-Henkle [mailto:va...@grameenfoundation.org]
Sent: Thursday, November 12, 2009 6:58 AM
To: Rao, Chandan; mifos-d...@lists.sourceforge.net
Cc: Thandavarayan, Parthasarathy; Pandiyarajan, Deepak; Kollam, Rajesh;
Adam Feuer
Subject: RE: [Mifos-developer] Next steps with the GK client family
detailsfeature
Jeff Brewster
>2567 as fixed.
Great job Chandan. Once this feature is merged to the trunk, I'll be
verifying your changes.
Also, we assigned a couple of more bugs to you today, so please check
your assigned list in issue tracker.
Regards,
Jeff
Adam Monsen
All of branches/sungard has been merged into trunk.
29. Nice work referring to issues fixed in commit log messages. When
resolving an issue as fixed, include information about where to look
for the actual fix. For example: "fixed in branches/sungard, r15000".
This helps during testing, verification, and forensics. Additionally,
it is helpful to include a link to mailing list discussions about the
issue. For example: "see http://tinyurl.com/yd2wg6e for related
discussion". Please revisit your recently resolved issues and include
this information. It is better to do many smaller commits rather than
fewer larger commits. Consider fixing one issue per commit.
30. In ClientCustActionForm, give meaningful names to variables. For
example: "setFamilyRelationship(int i, Short value)" ... the meaning
of "i" is not readily apparent, one must look to the usage of
setFamilyRelationship to derive intent.
Chand...@sungard.com
An update on the issues
All the issues have been fixed apart from
https://mifos.dev.java.net/issues/show_bug.cgi?id=2577
I can't seem to fix this issue, I have fixed the date fields getting
messed up but not the table borders not changing relatively. I think
this is a generic problem many screens in mifos, it is just that the
number of fields that I take in a row in the array is much higher than
in other screens, so the problem appears more glaring.
I found it in other screens too.. have attached one screenshot here
http://tinyurl.com/y884sbo
There is one test failing on Hudson. This seems totally unrelated with
what I was doing ( and my last commit, which was a harmless variable
name change) and all the tests do pass locally on my system. I wonder
what is wrong.
Regards
Chandan
-----Original Message-----
From: Adam Monsen [mailto:hai...@gmail.com]
Sent: Friday, November 13, 2009 5:57 AM
To: mifos-d...@lists.sourceforge.net
Subject: Re: [Mifos-developer] Next steps with the GK
clientfamilydetailsfeature
Jeff Brewster
>messed up but not the table borders not changing relatively. I think
>this is a generic problem many screens in mifos, it is just that the
>number of fields that I take in a row in the array is much higher than
>in other screens, so the problem appears more glaring.
>
>I found it in other screens too.. have attached one screenshot here
>http://tinyurl.com/y884sbo
>
I agree this is a more general bug Chandan. I closed 2577 and opened a
new issue, https://mifos.dev.java.net/issues/show_bug.cgi?id=2584, to
address this general alignment bug.
Thanks,
Jeff
Van Mittal-Henkle
> There is one test failing on Hudson. This seems totally unrelated with
> what I was doing ( and my last commit, which was a harmless variable
> name change) and all the tests do pass locally on my system. I wonder
> what is wrong.
This is an intermittent failure that is unrelated to your changes. I
forced a rebuild (with no changes) and it built successfully.
--Van