Bug 393168 - [Patch] Fix working week in 5.x/master
Summary: [Patch] Fix working week in 5.x/master
Status: RESOLVED FIXED
Alias: None
Product: kmymoney
Classification: Applications
Component: general (show other bugs)
Version: git (master)
Platform: Other All
: NOR normal
Target Milestone: ---
Assignee: KMyMoney Devel Mailing List
URL:
Keywords:
: 376071 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-04-15 16:20 UTC by learneroc
Modified: 2018-10-13 11:47 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 5.0.2


Attachments
Working week patch (2.99 KB, text/plain)
2018-04-15 16:20 UTC, learneroc
Details

Note You need to log in before you can comment on or make changes to this bug.
Description learneroc 2018-04-15 16:20:27 UTC
Created attachment 112047 [details]
Working week patch

Hello developers,

First of all, thank you all for this great piece of software!

(I've been mostly a user of KMyMoney, and just started to learn about coding recently.  It's my first time submitting a bug report (and a proposed patch), so kindly please bear with me if there's any infelicities in my expression)

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.

That's working in 4.8.  Out of curiosity I tried to find out what's changed.

It turns out there seems to be an 'overlooked' word in the porting of 'working week' to kf5, namely that:

-  // TODO: port kf5
-  int weekStart = 1;//KLocale::global()->workingWeekStartDay();
-  int weekEnd = 7;//KLocale::global()->workingWeekEndDay();
+  QLocale locale;
+  int weekStart = locale.firstDayOfWeek();
+  int weekEnd = weekStart-1;
+  if (weekEnd < Qt::Monday) {
+    weekEnd = Qt::Sunday;
+  }

well in fact, weekStart and weekEnd would be referring to the workingWeek, so in most cultures that'll actually be 1 and 5 (i.e. Monday to Friday).

In searching for alternatives (in porting away from KLocale in its wake), I found from my Google searches QLocale::weekdays() (as opposed to weekends) which seems to be a good replacement -- from the QT docs:

Returns a list of days that are considered weekdays according to the current locale.

So in the proposed patch attached I've set m_processingDays accordingly from QLocale::weekdays().  I've also moved down the #ifdef in KMyMoneyApp::isProcessingDate so that it could always check against weekdays/ends even without KF5Holidays.

Sorry for the lengthy description  (And don't be mistaken, there's not a scant of criticism at all.  To the contrary I appreciate all the developers' efforts all along, and the KF5 port must have been a little bit tedious)

And actually I'm very green at coding.   Very much appreciated if interested developer(s) could help review the proposed patch --- and feel free to amend / enhance it in any way you like.    

* * *

Some related past commits below for your reference:

2017-09-13  Ported start of week handing to KF5
(https://cgit.kde.org/kmymoney.git/commit/?id=e3aafecfbd096f048dcd53ae80f43f10e935c40e)

2016-06-16  Remove kdelibs4support, disbaled some code which will need to be ported.
(https://cgit.kde.org/kmymoney.git/commit/?id=fcfe5beca1656092d6f45495816e1b604427279c)

The feature (and the m_processingDays bitArray) was first introduced some 8 years ago:
2010-01-17  Added patch provided by Ian Neal
(https://cgit.kde.org/kmymoney.git/commit/?id=406cd4845f3b4685194142746a93a2720293c0be)
For reference, the related discussion about the implementation could be found in FEATURE: 221800
(https://bugs.kde.org/show_bug.cgi?id=221800)
Comment 1 Thomas Baumgart 2018-04-15 20:23:24 UTC
Cool, I think I came across this problem today and wondered, why a scheduled transaction did move from a Saturday to a Sunday instead of the expected Monday when I turned on the option to move to the next working day if it falls on a weekend. Your findings might explain that.

Since you are new to the KMyMoney development, I want to direct you to our phabricator site which allows you and us to do the review in a more elegant way as here on b.k.o.

Get yourself a KDE identity on https://identity.kde.org/index.php?r=registration/index and visit https://phabricator.kde.org/project/view/167/ and upload your patch there. It would help, if you also leave the description there.

In case you have more questions, please let us know on the developer mailing list at kmyoney-devel@kde.org.
Comment 2 Thomas Baumgart 2018-04-29 12:22:01 UTC
Git commit 983a33b786a20b503fb55a6411784ef64df5f895 by Thomas Baumgart.
Committed on 29/04/2018 at 12:22.
Pushed by tbaumgart into branch 'master'.

Fix working week

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.

Reviewers: #kmymoney, christiand, tbaumgart

Reviewed By: #kmymoney, christiand, tbaumgart

Subscribers: tbaumgart, christiand

Tags: #kmymoney

Differential Revision: https://phabricator.kde.org/D12412

M  +4    -13   kmymoney/kmymoney.cpp
M  +1    -0    kmymoney/mymoney/mymoneyschedule.cpp
M  +2    -0    kmymoney/mymoney/tests/mymoneyschedule-test.cpp

https://commits.kde.org/kmymoney/983a33b786a20b503fb55a6411784ef64df5f895
Comment 3 Thomas Baumgart 2018-04-29 12:30:04 UTC
Git commit 1c8a63c85e8d4280e78ab0cd245f6dae279e95ca by Thomas Baumgart.
Committed on 29/04/2018 at 12:29.
Pushed by tbaumgart into branch '5.0'.

Fix working week

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.
FIXED-IN: 5.0.2

Reviewers: #kmymoney, christiand, tbaumgart

Reviewed By: #kmymoney, christiand, tbaumgart

Subscribers: tbaumgart, christiand

Tags: #kmymoney

Differential Revision: https://phabricator.kde.org/D12412

(cherry picked from commit 983a33b786a20b503fb55a6411784ef64df5f895)

M  +4    -13   kmymoney/kmymoney.cpp
M  +1    -0    kmymoney/mymoney/mymoneyschedule.cpp
M  +2    -0    kmymoney/mymoney/tests/mymoneyschedule-test.cpp

https://commits.kde.org/kmymoney/1c8a63c85e8d4280e78ab0cd245f6dae279e95ca
Comment 4 Thomas Baumgart 2018-10-13 11:47:46 UTC
*** Bug 376071 has been marked as a duplicate of this bug. ***