Fix working week
ClosedPublic

Authored by ocoole on Apr 21 2018, 2:34 PM.

Details

Summary

There's a feature in scheduled transactions which allow them to move backwards/forwards in case a scheduled entry falls on a non-processing day, e.g. a weekend. However, I've noticed that that seems to have stopped working in 5.x/master. This change brings back this functionality.

BUG: 393168

Diff Detail

Repository
R261 KMyMoney
Lint
Lint Skipped
Unit
Unit Tests Skipped
ocoole requested review of this revision.Apr 21 2018, 2:34 PM
ocoole created this revision.
christiand accepted this revision as: christiand.Apr 21 2018, 9:05 PM
christiand added a subscriber: christiand.
christiand added inline comments.
kmymoney/kmymoney.cpp
1700

for (auto const& weekDay: locale.weekdays()) — saves a line.

This revision is now accepted and ready to land.Apr 21 2018, 9:05 PM
christiand requested changes to this revision.Apr 21 2018, 9:08 PM
christiand added inline comments.
kmymoney/kmymoney.cpp
4231

If KF5Holidys_FOUND is not set, the function does not return for non-processing dates.

This revision now requires changes to proceed.Apr 21 2018, 9:08 PM
ocoole updated this revision to Diff 32761.Apr 22 2018, 4:37 AM

Thank you for reviewing the patch, @christiand.
(Sorry for the newbie mistakes.)
I've updated the diff according to your comments.

ocoole marked 2 inline comments as done.Apr 22 2018, 4:39 AM
christiand accepted this revision.Apr 22 2018, 9:21 AM

Did not test, but looks good! Can you push yourself?

This revision is now accepted and ready to land.Apr 22 2018, 9:21 AM

Thanks Chris!
I don't think I have permission to push, but that's okay, I think it can wait before it's committed to the codebase.

(Um, I almost forgot — I actually posted this as a bug in 393168, before Thomas pointed me to Phabricator; that bug report could be closed when it is pushed.)

tbaumgart edited the summary of this revision. (Show Details)Apr 29 2018, 10:06 AM
tbaumgart requested changes to this revision.Apr 29 2018, 10:09 AM
tbaumgart added a subscriber: tbaumgart.

I was about to land it but found these two little nitpicks in your todo entries. I also updated the summary which should address the link to the bug entry.

kmymoney/mymoney/mymoneyschedule.cpp
1537

Can you turn

// TODO : ....

into

/// @todo ....

which allows doxygen to pick them up? Thanks.

kmymoney/mymoney/tests/mymoneyschedule-test.cpp
1590

Same here.

This revision now requires changes to proceed.Apr 29 2018, 10:09 AM
ocoole updated this revision to Diff 33282.Apr 29 2018, 11:29 AM

Thanks a lot, Thomas. Didn't think of doxygen before, so thanks for pointing that out! (actually it's my first time heard of it, I probably should check that out),

BTW I actually put down the todos a bit quite casually, so I didn't know if they make sense. Do feel free to let me know if you'd like any amendment to its wordings, or say, to remove one or two of them – they're open for suggestions.

ocoole marked 2 inline comments as done.Apr 29 2018, 11:31 AM
tbaumgart accepted this revision.Apr 29 2018, 12:19 PM

I'll take care of landing it now.

This revision is now accepted and ready to land.Apr 29 2018, 12:19 PM
This revision was automatically updated to reflect the committed changes.
ocoole added a comment.May 3 2018, 2:35 PM

Thanks Thomas!