Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Reverse payment history #12023

Closed

Conversation

josiah-keller
Copy link
Contributor

Fixes #11300

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

  1. Build from source
  2. Run npm run add-simulated-payment-history 50
  3. Open payment history, note that order is now newest to oldest

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@codecov-io
Copy link

codecov-io commented Nov 18, 2017

Codecov Report

Merging #12023 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12023      +/-   ##
==========================================
+ Coverage   53.38%   53.39%   +<.01%     
==========================================
  Files         274      274              
  Lines       26018    26015       -3     
  Branches     4170     4170              
==========================================
  Hits        13890    13890              
+ Misses      12128    12125       -3
Flag Coverage Δ
#unittest 53.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...renderer/components/preferences/payment/history.js 38.75% <ø> (+1.4%) ⬆️

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right solution. It would fix the case reported, but users that have been accumulating a history will have it in the expected order. What needs to be done is to actually sort based on the date descending.

For an example, here's what my screen looks like with our current release (0.19.95):
screen shot 2017-11-20 at 9 03 52 am

This PR would break that ordering. I'll update the original issue to mention how it needs sorting if you wanted to re-attempt the PR? 😄

@josiah-keller
Copy link
Contributor Author

@bsclifton Heh, whoops. I'll bet the original issue was actually supposed to be that the test data is backwards and I just misread it. Wouldn't the better solution be to fix simulateLedgerTransactions.js?

@bsclifton
Copy link
Member

bsclifton commented Nov 20, 2017

@josiah-keller that would work too- although it would be better if it was properly sorted. Shouldn't be too hard, I would suspect something like this:

let transactions = Immutable.fromJS(
  addExportFilenamePrefixToTransactions(this.props.ledgerData.get('transactions').toJS())
)
transactions = transactions.sort((a, b) => {
  if (date a is less than b) {
    return 1
  }
  if (date a is greater than b) {
    return -1
  }
  return 0
})

@bsclifton
Copy link
Member

@josiah-keller I'm going to close this PR for now... but please do re-open if you have a chance to revisit and try a fix, per the above 😄 I'd love to review (if not, it's all good too)

Best wishes and Happy Thanksgiving 🦃

@bsclifton bsclifton closed this Nov 22, 2017
@bsclifton bsclifton removed this from the Triage Backlog milestone Nov 22, 2017
@josiah-keller josiah-keller changed the title Reverse payment history order Sort payment history order by date Nov 23, 2017
@josiah-keller josiah-keller changed the title Sort payment history order by date Reverse payment history Nov 23, 2017
@josiah-keller
Copy link
Contributor Author

@bsclifton Yeah, sorry I haven't had a chance to get to it until now. Since I squashed and force-pushed, it seems I can't reopen the PR, so I'll open a new one. Thanks and happy Thanksgiving to you as well.

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

Successfully merging this pull request may close these issues.

4 participants