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

feat: updateOrAddMeta for wallet #767

Closed
wants to merge 2 commits into from

Conversation

justinkekeocha
Copy link
Contributor

It will be nice to have a method that updates or adds a new key to the meta.

Just like what you did here: https://github.com/bavix/laravel-wallet/issues/498#issuecomment-1124005291

Verified

This commit was signed with the committer’s verified signature. The key has expired.
phansch Phil Hansch
It will be nice to have a method that updates or adds a new key to the meta.

Just like what you did here: bavix#498 (comment)

Signed-off-by: Kekeocha Justin Chetachukwu <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
changed variable name

Signed-off-by: Kekeocha Justin Chetachukwu <[email protected]>
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #767 (d87ea69) into master (bd9b73b) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

❗ Current head d87ea69 differs from pull request most recent head 14be368. Consider uploading reports for the commit 14be368 to get more accurate results

@@              Coverage Diff              @@
##              master     #767      +/-   ##
=============================================
- Coverage     100.00%   99.89%   -0.11%     
- Complexity       551      552       +1     
=============================================
  Files             83       83              
  Lines           1946     1948       +2     
=============================================
  Hits            1946     1946              
- Misses             0        2       +2     
Files Changed Coverage Δ
src/Models/Wallet.php 93.93% <0.00%> (-6.07%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rez1dent3
Copy link
Member

Thank you for your contribution to the project, but I can't add this feature to the wallet model.

This functionality should be located in the area of the team that is developing its software product. Here's how to define replace or merge at the package level? How to solve a problem with a depth greater than 1?

In addition, the name of the method is not correct. By update, people understand an sql operation, because this method is in the model.

Here is a small example for you:

$meta = [
	'order' => [
		'id' => '92e330ec-c248-48a7-b7c2-a7d48535e417',
		'items' => [1,2,3,4,15],
	],
];

$meta = array_merge($meta, [
	'order' => [
		'title' => 'Message',
	],	
]);

/*
array(1) {
  ["order"]=>
  array(1) {
    ["title"]=>
    string(7) "Message"
  }
}
*/

@justinkekeocha
Copy link
Contributor Author

justinkekeocha commented Sep 12, 2023

The code above does not remove existing records when saved if that is what you mean. Maybe you should actually run the method.

@rez1dent3
Copy link
Member

The code above does not remove existing records when saved if that is what you mean. Maybe you should actually run the method.

I see and understand, I'm talking about the name and area of responsibility. The formation of a meta array is not the responsibility of the package.

@justinkekeocha
Copy link
Contributor Author

Name can be changed to what will best describe it. I was just trying to add more helpers like the Eloquent updateOrCreate() method.

@rez1dent3
Copy link
Member

rez1dent3 commented Sep 12, 2023

I understand, but this is not the responsibility of the package. Here you need to approach conservatively, according to the principle of YAGNI (You Aren't Gonna Need It). The package develops from the feedback received, this functionality is superfluous. The more functions we add, the more difficult and expensive refactoring is. As part of the package, we do not solve the problems that developers have with php. Here we are developing an API for working on the wallet.

If you are looking for useful functionality, then please develop the balance with credit method. The point is to display the full balance along with the credit limit. You can read about the functionality here: https://bavix.github.io/laravel-wallet/#/credit-limits

UPD: Only necessarily new functionality must be covered by unit tests.

@justinkekeocha
Copy link
Contributor Author

I tried that, but it didn't work, which was was what set me searching.

@rez1dent3
Copy link
Member

Sorry, I didn't catch the context. Can you tell me more about what you're talking about?

@justinkekeocha
Copy link
Contributor Author

Quick question: supposing I want to extend the wallet.php model to add the feature for my own custom app. How would I do this. From going through the codebase, the MorphOne trait, has a wallet method which calls the wallet model. The getWalletAttribute() also calls a CastServiceInterface.

But I couldn't find a way to extend the model.

@rez1dent3
Copy link
Member

rez1dent3 commented Sep 13, 2023

@justinkekeocha You need to expand the model and replace it through the config.

'model' => Wallet::class,

Example MyWallet.php:

use Bavix\Wallet\Models\Wallet as WalletBase;

class MyWallet extends WalletBase {
    public function helloWorld(): string { return "hello world"; }
}

config/wallet.php

...
    'wallet' => [
        'table' => 'wallets',
        'model' => MyWallet::class,
        'creating' => [],
        'default' => [
            'name' => 'Default Wallet',
            'slug' => 'default',
            'meta' => [],
        ],
    ],
...
echo $user->wallet->helloWorld();

UPD: Even now I’ll try to write a unit test for this and attach it below.

@rez1dent3
Copy link
Member

@rez1dent3
Copy link
Member

I don't see any benefit for the package with this PR, I'm really sorry. I'm closing the pull request.
Thank you for your contribution to the development of the project.

@rez1dent3 rez1dent3 closed this Sep 13, 2023
@rez1dent3
Copy link
Member

Can you please document this expansion, I really struggled to expand this model and I believe it is the same with other. If this will be much of a hassle for you, you can recommend which page I can put it in the documentation

@justinkekeocha It seems to me that a new section in the documentation needs to be created for this. If you have a desire, please add.

@justinkekeocha
Copy link
Contributor Author

I have done this here : https://github.com/bavix/laravel-wallet/pull/770

Is there a way to extend the Transaction and Transfer class?

I am guessing it is editing this

` /**
* Base model 'transaction'.
*/
'transaction' => [
'table' => 'transactions',
'model' => Transaction::class,
],

/**
 * Base model 'transfer'.
 */
'transfer' => [
    'table' => 'transfers',
    'model' => Transfer::class,
],`

If yes, then let me go forward with the same way I did with the Wallet class

@rez1dent3
Copy link
Member

Yes you are right. It's similar there.

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

2 participants