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

fix: prevent amount to be truncated on transactions headers #851

Merged
merged 18 commits into from
Jun 22, 2021

Conversation

alfonsobries
Copy link
Member

@alfonsobries alfonsobries commented Jun 17, 2021

Summary

https://app.clickup.com/t/mfbyf4

Refactor the headers for the transactions that have amounts to prevent the amounts to be truncated.

To test:

You can use this transaction that have a large amount /transactions/3bebe1af7558b8ad94c873e951a0d156ab82941da27b85c6a3e1e9226cf5b106

Also, you can open one transaction per type by filtering on the home page and opening one per type. Is not easy to find transactions with large amounts so for testing purposes you can hardcode a random amount on the app/ViewModels/TransactionViewModel.php amount() method

image

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

}

return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

would it make more sense to reverse this, or reducing it completely until there are more "amount" types? 🤔

    public function hasAmount(): bool
    {
        if ($this->isTransfer()) {
            return true;
        }

        if ($this->isMultiPayment()) {
            return true;
        }

        return false;
    }

or

    public function hasAmount(): bool
    {
        return $this->isTransfer() || $this->isMultiPayment();
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

@alexbarnsley by checking the views that don't have the amount I was able to securely say which types don't have the amount, the rest of them used the fallback view and includes other types like "second signature", IPFS, etc. In other words, those types are the ones that have a special template so it's safest to exclude them. (In case you are wondering I tested the condition you proposed and even that the test pass some views are broken)

Regarding the multiline, it was in purpose to ensure the code coverage covers all the conditions

Copy link
Member

Choose a reason for hiding this comment

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

@alfonsobries can you add a comment regarding that to the code so the next guy doesn't go and refactor this without prior knowledge

@alexbarnsley alexbarnsley self-assigned this Jun 17, 2021
@ItsANameToo ItsANameToo marked this pull request as draft June 18, 2021 13:10
alfonsobries and others added 3 commits June 18, 2021 15:23
Co-authored-by: Alex Barnsley <8069294+alexbarnsley@users.noreply.github.com>
…blade.php

Co-authored-by: Alex Barnsley <8069294+alexbarnsley@users.noreply.github.com>
@alfonsobries alfonsobries marked this pull request as ready for review June 18, 2021 15:37
@alfonsobries alfonsobries changed the title fix: prevent amount to be truncate on transactions headers fix: prevent amount to be truncated on transactions headers Jun 21, 2021
}

return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

@alfonsobries can you add a comment regarding that to the code so the next guy doesn't go and refactor this without prior knowledge

tests/Unit/ViewModels/TransactionViewModelTest.php Outdated Show resolved Hide resolved
@ItsANameToo ItsANameToo marked this pull request as draft June 22, 2021 08:10
@alfonsobries alfonsobries marked this pull request as ready for review June 22, 2021 11:04
@alfonsobries alfonsobries marked this pull request as draft June 22, 2021 11:04
@alfonsobries alfonsobries marked this pull request as ready for review June 22, 2021 11:20
@ItsANameToo ItsANameToo merged commit 6b4f2e1 into develop Jun 22, 2021
@ItsANameToo ItsANameToo deleted the fix/prevent-amount-truncate branch June 22, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants