[Kmymoney-devel] Review Request: show another icon on homepage accounts tables if there are transactions after the last online transaction

[Marko Käning]

> On Jan. 3, 2013, 6:55 p.m., Łukasz Maszczyński wrote:
> > kmymoney/mymoney/mymoneyfiletest.cpp, lines 2124-2136
> > <http://git.reviewboard.kde.org/r/107619/diff/7/?file=104083#file104083line2124>
> >
> >     What is the purpose of all the QVERIFY-s here? (Same in the next test)

The purpose is - to my understanding - that only expected actions of the object instance make the test pass successfully.

I copied these more or less from other tests
 and
  I have no idea whether it would be safe enough to do without them here... (I guess I need to add Alvaro, Thomas and Cristian as reviewers here too.)


> On Jan. 3, 2013, 6:55 p.m., Łukasz Maszczyński wrote:
> > kmymoney/mymoney/mymoneyfiletest.cpp, lines 2113-2118
> > <http://git.reviewboard.kde.org/r/107619/diff/7/?file=104083#file104083line2113>
> >
> >     In my opinion it's safe to assume that addSplit() won't throw, so I would skip the try-catch block here, as it makes the test a bit less readable. But it's also ok the way it is. (Same in the next test

Well, perhaps you are right, since similar tests occur earlier up in the test sequence.


> On Jan. 3, 2013, 6:55 p.m., Łukasz Maszczyński wrote:
> > kmymoney/mymoney/mymoneyfiletest.cpp, line 2106
> > <http://git.reviewboard.kde.org/r/107619/diff/7/?file=104083#file104083line2106>
> >
> >     Does this impact the test at all? E.g. would it matter if you changed "Memotext" to something else or removed the line? (Applies to the next test as well)

We can do without it, I think. It's just a residue.


> On Jan. 3, 2013, 6:55 p.m., Łukasz Maszczyński wrote:
> > kmymoney/mymoney/mymoneyfiletest.cpp, lines 2084-2085
> > <http://git.reviewboard.kde.org/r/107619/diff/7/?file=104083#file104083line2084>
> >
> >     Extracting a hardcoded value (2011,12,1) to a variable (tDate) is a good idea - it improves readability, and the same object can be used many times if necessary.
> >     
> >     Now think of a better variable name so that the comment will no longer be needed (same applies to remaining two tests).

Good point.
I'll do something about it.

(The two issues at the bottom I won't touch until I get feedback from the other devs.)


- Marko


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107619/#review24595
-----------------------------------------------------------


On Jan. 2, 2013, 10:49 p.m., Marko Käning wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107619/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2013, 10:49 p.m.)
> 
> 
> Review request for KMymoney and Łukasz Maszczyński.
> 
> 
> Description
> -------
> 
> The homepage accounts tables now show a different icon if there are transactions for an account after the last online transaction - highlighting for the user the necessity to download transactions in order to verify the account status.
> 
> 
> Diffs
> -----
> 
>   kmymoney/mymoney/mymoneyfile.h c43977ce7413eee2bc1e0a841fa548314c71e9df 
>   kmymoney/mymoney/mymoneyfile.cpp 6640356d5e07152f8eb4aecff23d62ee8d853dbd 
>   kmymoney/mymoney/mymoneyfiletest.h 5551fa94b6b34022c8e91c0301847d5479e6b1f2 
>   kmymoney/mymoney/mymoneyfiletest.cpp feb9d57ecd7156fc1eea189a905fa797aeae9183 
>   kmymoney/views/khomeview.cpp c79337176b7265cabe15cad4972bc719e797af7c 
> 
> Diff: http://git.reviewboard.kde.org/r/107619/diff/
> 
> 
> Testing
> -------
> 
> Built and ran application and test cases successfully.
> 
> 
> Screenshots
> -----------
> 
> example for the 4 possible cases of online status
>   http://git.reviewboard.kde.org/r/107619/s/948/
> 
> 
> Thanks,
> 
> Marko Käning
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20130103/aa83fad9/attachment-0001.html>