Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows 1.5.21 has calculation error in account view #5122

Closed
mike9665 opened this issue Sep 23, 2022 Discussed in #5121 · 25 comments
Closed

Windows 1.5.21 has calculation error in account view #5122

mike9665 opened this issue Sep 23, 2022 Discussed in #5121 · 25 comments
Assignees
Milestone

Comments

@mike9665
Copy link

Discussed in #5121

Originally posted by mike9665 September 23, 2022
I updated from 1.5.20 to 1.5.21 and a deposit dated today is not included in the balance. If I change the date to yesterday the balance is correct. I have also fallen back to 1.5.20 and the balance is correct when the deposit is dated today.
Mike

@whalley
Copy link
Member

whalley commented Sep 23, 2022

When you say 'not included in the balance' what do you mean?

CleanShot 2022-09-23 at 08 34 17

Is it the balance at A or B in above screenshot. The value at B is dependant on how you sort the transaction view.

@whalley whalley added this to the v1.5.22 milestone Sep 23, 2022
@mike9665
Copy link
Author

mike9665 commented Sep 23, 2022 via email

@whalley
Copy link
Member

whalley commented Sep 23, 2022

As noted, the balance displayed will differ depending upon sort. Descending and Ascending date will have different results. With descending date the first item (latest date) will hold the balance, and with ascending date the last item (latest date) will hold the balance. Is this what you are seeing.

Descending date
CleanShot 2022-09-23 at 09 05 42

Ascending date
CleanShot 2022-09-23 at 09 06 09

@mike9665
Copy link
Author

mike9665 commented Sep 23, 2022 via email

@whalley
Copy link
Member

whalley commented Sep 23, 2022

Can you please share a screen shot of what the issue is, I'm struggling to help without this.

@whalley
Copy link
Member

whalley commented Sep 23, 2022

It may be related to a 'sort refresh' error (#5108) in the current build.... Does the build here help?
https://ci.appveyor.com/project/moneymanagerex/moneymanagerex/builds/44859308/job/v3t4q2w20wm4me62/artifacts

@mike9665
Copy link
Author

mike9665 commented Sep 23, 2022 via email

@whalley
Copy link
Member

whalley commented Sep 23, 2022

Images not visible. Please don't respond by mail. Can you post here: #5122

@mike9665
Copy link
Author

mike9665 commented Sep 23, 2022

I have installed the beat version 1.5.22 and the behavior is unchanged.

snip1 This is the view and sort I normally use. The true balance value is 1482.44 for September 23rd
snip1

snip2 I have move the £175.00 credit entry to September 22 and the balance is now correct.
snip2

snip3 I have restored the credit balance date to September 23 and move the £10 withdrawal entry to September 22 and the balance is correct.
snip3

snip4 The 4th screen snip is the date,ascending view with the two transactions I edited above from today restored to todays date.
snip4

@mike9665
Copy link
Author

picture 1 is snip3
picture 2 is snip2
picture 3 is snip1
picture 3 is snip4

@whalley
Copy link
Member

whalley commented Sep 23, 2022

Have reordered the images!

@n-stein
Copy link
Contributor

n-stein commented Sep 23, 2022

I think the issue is the balance shown in the list doesn't take into account the sort order of the secondary sort column.

Example: If you put a primary sort by "Date" descending and a secondary sort by "ID" ascending you run into the issue where the top record for a given date doesn't show the current account balance.
image

@whalley
Copy link
Member

whalley commented Sep 23, 2022

@n-stein Yes, this maybe indeed be the case. @mike9665 has the ID field hidden. There was some discussion around users possibly being confused by the secondary sort when we originally looked at this. #4690.

Maybe we:

  1. Provide an option (default=OFF) for supporting the secondary sort functionality - most people will leave it as-is.
  2. When DATE is the primary sort key then secondary is ALWAYS set to ID in the SAME sort direction

I favour option (2).

@n-stein
Copy link
Contributor

n-stein commented Sep 23, 2022

@whalley I think option 2 would work, but breaks the consistency of the "Secondary Sort" behavior. You couldn't achieve the below view where secondary sort is by Payee and primary sort is by date:
image

What about changing the behavior of the Model_Account::transaction method to sort by secondary sort order followed by date instead of the default ID then date here?

std::sort(trans.begin(), trans.end());
std::stable_sort(trans.begin(), trans.end(), SorterByTRANSDATE());

@whalley
Copy link
Member

whalley commented Sep 23, 2022

I feel the main issue here is with users who don't need, or more likely don't realise, that the secondary sort function exists.

They just click DATE and assume that things are sorted in chronological order which means DATE (and ID to put some order to the the IDs on the same date).

In your example with DATE/PAYEE order it is going to cause confusion as the Balance against ID 53 does not match the headline 'Account Bal:'. Note this DATE/ID order is used across the application to represent the account balance.

@n-stein
Copy link
Contributor

n-stein commented Sep 23, 2022

I think the DATE/PAYEE order is only confusing right now specifically because ID 53 does not match the headline balance. If the balance was calculated by DATE/PAYEE as displayed rather than DATE/ID then 53 would contain the headline balance, no?

I see how changing the DATE/ID function of Model_Account::transaction could cause issues due to its wide usage. We could instead include the secondary and primary sort between these two lines:

const auto i = isAllAccounts_ ? Model_Checking::instance().all() : Model_Account::transaction(this->m_account);
for (const auto& tran : i)

@whalley
Copy link
Member

whalley commented Sep 23, 2022

I'll let others contribute to the discussion. I'm a little wary about changing the balance calculations as it may cause further confusion. I think there is simplicity/comfort in knowing that the balance line against a transaction remains constant regardless of sorting.

@whalley
Copy link
Member

whalley commented Sep 25, 2022

Having re-read #4690, I'm now more convinced that sorting on Date should sort by ID (as the secondary) also. @vomikan @tactilis views? I feel the 'balance' field should always attempt to reflect what the user would see on a bank statement. (maybe even more accurate when we add a 'time' field #2729

@tactilis
Copy link

I'm now more convinced that sorting on Date should sort by ID (as the secondary) also.

We did discuss this along the way. It is the logical thing to do because, as I assert here #4690 (comment), I think relatively few people have a need to do anything other than the standard Date | ID sort.

I still think the idea of having a visual indication of the secondary sort column would be helpful. Maybe not the dashed up/down arrow suggested previously but some way of knowing what the secondary column is - perhaps a tooltip that is shown when you hover over the primary sort column header?

Not sure what's for the best. 1.5.21 sorting has stirred up a lot of comments!

whalley pushed a commit to whalley/moneymanagerex that referenced this issue Sep 25, 2022
@whalley
Copy link
Member

whalley commented Sep 25, 2022

Not sure if we can add tooltips to the column header.

What about adding some sort information to the header panel?
CleanShot 2022-09-26 at 00 46 38

whalley pushed a commit to whalley/moneymanagerex that referenced this issue Sep 26, 2022
whalley added a commit that referenced this issue Sep 26, 2022
@grumpy235
Copy link

First time I displayed my transactions using my default selection of "View all" and this fix
Screenshot 2022-09-27 072120
So I get Date/Date which is perhaps a little confusing for some - it was for me :-)
When I click Date column to force a sort then I get
Screenshot 2022-09-27 073001
as expected

whalley pushed a commit to whalley/moneymanagerex that referenced this issue Sep 27, 2022
@whalley
Copy link
Member

whalley commented Sep 27, 2022

@grumpy235 Good spot, the secondary sort default before any setting is saved was also DATE. Have now set to ID.

whalley added a commit that referenced this issue Sep 27, 2022
fix(#5122): secondary sort default is ID
@PMaff
Copy link
Contributor

PMaff commented Sep 27, 2022

I do think that the balance is only correct, if the items are ordered by date (low-> high date or high-> low date).
I always sort by date to have the last transaction on top.

So maybe we can hide the balance column as soon as the sort is not done on date?
Or enter some hint, as soon as not all transactions are shown or the order is not done by date (1.5.21):
Balancehint

The secondary sort arrow e.g. does not
show on the top of the ID column (1.5.22beta) but this may also confuse:
Sorting1_5_22beta

Is it possible to switch off secondary sort completely?
Where is

  • sort by Date
  • sort by Payee (secondary sort is now Date)
  • sort by ID (secondary sort is now Payee)
  • sort by Payee (secondary sort is now by ID)

described?

@whalley
Copy link
Member

whalley commented Sep 29, 2022

The balance is ALWAYS calculated as on a bank statement, i.e. date (or more strictly DATE/ID) order. Having the balance displayed even when not sorting by date can often be useful as it still represents the balance when the transaction was posted.

The secondary sort on teh column headers was not implemented to avoid confusion, it just shows the last (primary) sort order selected by the user. Having the primary and secondary sort displayed alongside the transaction view information gives further information.

Is it possible to switch off secondary sort completely?

No.

Where is ..... described?

Where is anything described? We have no documentation that is kept up-to-date, there is no resource to do it. It would be great to have a technical author to write all this.

@Patricen1
Copy link

It looks like the issue is not closed within 1.6.0 version.
Within account view, some withdrawal operations are added to the balancewhile some of the deposit operations are substracted from the balance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants