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

Add support for transaction note #10

Merged
merged 17 commits into from
Jan 11, 2024

Conversation

3m1n3nc3
Copy link
Contributor

@3m1n3nc3 3m1n3nc3 commented Jan 4, 2024

  1. There are times when you want to ensure that only specific wallets are charged during a transaction, this PR adds support for optionally specifying which wallets should be charged in such scenarios, instead of attempting to charge all wallets.
    A possible use case for this is in an escrow system where funds needs to stay locked in untill an approval is given.

  2. This PR also adds support for including short transactional notes, possibly to provide context to the user.

Both features are covered by all possible test cases.

See Issue #8

@HPWebdeveloper
Copy link
Owner

HPWebdeveloper commented Jan 6, 2024

@3m1n3nc3

I initially thought of adding a transaction note, but it wasn’t implemented at the time. Thank you for your contribution.

For this specific pull request, our focus will be exclusively on Purpose No. 2, namely the addition of the Transaction Note feature. After successfully merging this, we'll move forward with a new pull request for Purpose No. 1.

To advance with these changes, please follow these steps:

  • Rebase your fork with the main branch
  • Remove any code pertaining to Purpose No. 1.
  • When it is done, execute the command ./vendor/bin/pint for code formatting.
  • Since Feature No. 2 involves modifications to the table, It is required to update the README with instructions on how to implement these changes.
  • When it is done mention me to review

Thank you.

@HPWebdeveloper HPWebdeveloper marked this pull request as draft January 6, 2024 15:28
@HPWebdeveloper HPWebdeveloper changed the title Add support for restricted wallets and notes for logs. Add support for transaction note Jan 6, 2024
@HPWebdeveloper HPWebdeveloper added the feature Improvements or additions to documentation label Jan 6, 2024
@SSEsmaeeli SSEsmaeeli marked this pull request as ready for review January 6, 2024 17:06
@HPWebdeveloper HPWebdeveloper marked this pull request as draft January 6, 2024 22:53
@3m1n3nc3
Copy link
Contributor Author

3m1n3nc3 commented Jan 8, 2024

@3m1n3nc3

I initially thought of adding a transaction note, but it wasn’t implemented at the time. Thank you for your contribution.

For this specific pull request, our focus will be exclusively on Purpose No. 2, namely the addition of the Transaction Note feature. After successfully merging this, we'll move forward with a new pull request for Purpose No. 1.

To advance with these changes, please follow these steps:

  • Rebase your fork with the main branch
  • Remove any code pertaining to Purpose No. 1.
  • When it is done, execute the command ./vendor/bin/pint for code formatting.
  • Since Feature No. 2 involves modifications to the table, It is required to update the README with instructions on how to implement these changes.
  • When it is done mention me to review

Thank you.

@HPWebdeveloper It's all done now.

@3m1n3nc3 3m1n3nc3 marked this pull request as ready for review January 8, 2024 15:57
@HPWebdeveloper
Copy link
Owner

HPWebdeveloper commented Jan 11, 2024

@3m1n3nc3

The reference column is expected to be auto-filled, correct?

I used this Demo and forced its composer to use this branch in local. reference column in empty. I just published the pay-pocket config file without any change.

Screenshot 2024-01-11 at 11 34 39 AM

@3m1n3nc3
Copy link
Contributor Author

@3m1n3nc3

The reference column is expected to be auto-filled, correct?

I used this Demo and forced its composer to use this branch in local. reference column in empty. I just published the pay-pocket config file without any change.

Screenshot 2024-01-11 at 11 34 39 AM

I think I forgot to reimplement the feature after the rebase. Let me do that.

@3m1n3nc3
Copy link
Contributor Author

3m1n3nc3 commented Jan 11, 2024

@3m1n3nc3
The reference column is expected to be auto-filled, correct?
I used this Demo and forced its composer to use this branch in local. reference column in empty. I just published the pay-pocket config file without any change.
Screenshot 2024-01-11 at 11 34 39 AM

I think I forgot to reimplement the feature after the rebase. Let me do that.

Turned out I failed to add the reference field to the fillable property, I've done that now.

@HPWebdeveloper HPWebdeveloper merged commit 445bb28 into HPWebdeveloper:main Jan 11, 2024
14 checks passed
@HPWebdeveloper
Copy link
Owner

@3m1n3nc3

suppose user published the config file and doesn't provide a correct value for the 'log_reference_generator' => null,

what he provides is wrong and it is not invokable.

In this case there is a need to have Exceptions here:

https://github.com/HPWebdeveloper/laravel-pay-pocket/blob/main/src/Traits/BalanceOperation.php

Can you complete it?

@3m1n3nc3
Copy link
Contributor Author

@3m1n3nc3

suppose user published the config file and doesn't provide a correct value for the 'log_reference_generator' => null,

what he provides is wrong and it is not invokable.

In this case there is a need to have Exceptions here:

https://github.com/HPWebdeveloper/laravel-pay-pocket/blob/main/src/Traits/BalanceOperation.php

Can you complete it?

Currently it falls back to Str:random()
I will add an exception and submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants