----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130143/#review103274 -----------------------------------------------------------
Here are some things that came to my mind while reading through this review request. Unfortunatly I cannot try the patch because I cannot compile the 4.8 branch. What happens if the user creates a new account (no opening balances account) and enters a opening balance and there is no opening balances account, yet? Is a opening balance account created (silently)? The user can rename it later then (less user interaction, so this is my preferred behavior). What if the user creates a opening balances account with a opening balance? kmymoney/converter/mymoneytemplate.cpp (line 416) <https://git.reviewboard.kde.org/r/130143/#comment68695> I think setting a value is not needed. Also someone who only reads the XML file could think setting ```value="0"``` could deactivate the flag. Then it would be ```<flag name="OpeningBalanceAccount" />```. kmymoney/dialogs/knewaccountdlg.cpp (line 239) <https://git.reviewboard.kde.org/r/130143/#comment68700> This could confuse the user. Because he selected his new account to become an opening balances account but this option is silently ignored. To solve this the checkbox to make an account an opening balances account could be disabled and a notice or label inform the user why. kmymoney/dialogs/knewaccountdlg.cpp (line 243) <https://git.reviewboard.kde.org/r/130143/#comment68699> ```transactionList.isEmpty()``` is faster kmymoney/mymoney/mymoneyfile.cpp (line 1104) <https://git.reviewboard.kde.org/r/130143/#comment68697> The RegExp could be replaced by ```QString::startsWith()``` http://doc.qt.io/qt-5/qstring.html#startsWith kmymoney/mymoney/mymoneyfile.cpp (line 1107) <https://git.reviewboard.kde.org/r/130143/#comment68698> I know you did not write but I see it now: The ```if```s could be combinde with ```&&```? Also ```(*it).foobar()``` is ```it->foobar()```. - Christian David On Mai 30, 2017, 9:45 vorm., Ralf Habacker wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130143/ > ----------------------------------------------------------- > > (Updated Mai 30, 2017, 9:45 vorm.) > > > Review request for KMymoney. > > > Bugs: 370290 > http://bugs.kde.org/show_bug.cgi?id=370290 > > > Repository: kmymoney > > > Description > ------- > > This patch introduce a feature to specify a dedicated account > to be used as opening balance account instead of using an account > with a predefined name which may be language specific. > > The "opening balance account" flag could be set in the account > editor if no other account contains this flag. Also changing > the state of the flag is only possible if no transactions are > assigned to the account having this flag. > > On creating a new kmymoney file the "opening balance account" > flag is imported from a related template account flag if specified > in the following form > > <account type="16" name="9000 Saldovortragskonten"> > <flag name="OpeningBalanceAccount" value="1"/> > </account> > > If specified the template admin needs to make sure that only one > template account has this flag set. > > Exporting the current kmymoney file to an account template exports > this flag too. > > BUG:370290 > > > Diffs > ----- > > kmymoney/converter/mymoneytemplate.cpp > 25a343e3fbdd9409ebdbd814bc08122c151a09d9 > kmymoney/dialogs/knewaccountdlg.cpp > dfe2967174bf323d9eda4983fd545d0930a9ec43 > kmymoney/dialogs/knewaccountdlgdecl.ui > bee638d2b4f73bc8496c86bbf606bfaa5fa4c913 > kmymoney/mymoney/mymoneyfile.cpp 692014b21ec8bff4e4c3f3f240d377cd7b9697b3 > kmymoney/mymoney/storage/mymoneystorageanon.cpp > b6d0dc6a7b499aa45498cb76bef836731ff618d4 > kmymoney/reports/objectinfotable.cpp > 584a9a378d37d51766e551d8e6b6baffe4fb397d > kmymoney/reports/reportstestcommon.h > 22000165dff793c5d7281072f702e0ca3c40f882 > kmymoney/reports/reportstestcommon.cpp > 40b103ca965e0a1973b6fd0a351ddb976aa10471 > > Diff: https://git.reviewboard.kde.org/r/130143/diff/ > > > Testing > ------- > > - compiled > - set/unset "opening balance account" flag in account editor for a given > account and save/load kmymoney file -> state is persistent > - checked if it is possible to set "opening balance account" flag in an > additional account -> check box is not visible on editing the second account > - checked if it is possible to change "opening balance account" flag if > transactions are assigned to the opening balance account -> check box is > disabled > > Note: I choosed the flag name as to be 'OpeningBalanceAccount' in the > template file and kmymoney file to have the same name. > > > Thanks, > > Ralf Habacker > >
