use correct paid_for_type on payment #396 #400

beesaferoot · 2024-12-08T20:05:17Z

seed meter tokens table for demo company
use appropriate paid_for_type as expected on the frontend

Brief summary of the change made

This PR seeds the meter_tokens table, which ensures the tokens field is correctly returned with values on the api/meters/{meter_id}/ endpoint.

Also on success payment, the payment history paid_for_type is updated to match expected values on the frontend.

Closes: #396
Closes: #83

Are there any other side effects of this change that we should be aware of?

We can now see the tokens on the transaction

Describe how you tested your changes?

Pull Request checklist

Please confirm you have completed any of the necessary steps below.

Meaningful Pull Request title and description
Changes tested as described above
Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.

dmohns · 2024-12-09T13:06:49Z

src/backend/app/Listeners/PaymentListener.php

@@ -86,11 +86,11 @@ public function onPaymentSuccess(
                $this->applianceRatePaymentHistoryService->assign();
                break;
            case $paidFor instanceof Asset:
-                $paymentHistory->paid_for_type = Asset::class;
+                $paymentHistory->paid_for_type = 'appliance';


This looks like a massive change. Probably it's correct, but I'm curious: How did you test this?

I checked on the frontend, the Meter Transanction list has a condition to display approriate message based on the paid for type.

<md-table-row v-for="token in transactions.tokens" :key="token.id"> <md-table-cell v-text="token.transaction.original_transaction_type" ></md-table-cell> <md-table-cell v-text="moneyFormat(token.transaction.amount)" ></md-table-cell> <md-table-cell v-if="token.paid_for_type === 'token'"> Token {{ token.paid_for.token }} </md-table-cell> <md-table-cell v-else> {{ token.paid_for_type }} </md-table-cell> <md-table-cell v-if="token.paid_for_type === 'token'" v-text="readable(token.paid_for.energy) + 'kWh'" ></md-table-cell> <md-table-cell v-else>-</md-table-cell> <md-table-cell v-text="timeForTimeZone(token.created_at)" ></md-table-cell> </md-table-row>

However, on the backend, the string is updated to the internal class name (i.e App\Model\Token), which did not match with what the frontend expected; I found that weird.

Also, while searching for the usecases for paid_for_type field I found this factory. The values seem to tally with the frontend expectation

class PaymentHistoryFactory extends Factory { protected $model = PaymentHistory::class; /** * Define the model's default state. * * @return array */ public function definition() { return [ 'id' => 1, 'transaction_id' => 1, 'amount' => $this->faker->randomFloat(2, 0, 100), 'payment_service' => $this->faker->randomElement(['vodacom_transaction', 'airtel_transaction', 'agent_transaction']), 'sender' => $this->faker->phoneNumber, 'payment_type' => $this->faker->randomElement(['appliance', 'energy', 'installment', 'access rate']), 'paid_for_type' => $this->faker->randomElement(['appliance', 'token', 'loan_rate', 'access_rate']), 'paid_for_id' => 1, 'payer_type' => 'person', 'payer_id' => 1, ]; } }

With the change, I needed to reseed the demo database with the new format. As shown below, using this format appears to correctly show tokens for transactions that are made for tokens.

What are your thoughts? Do you think we should have a different display for tokens? Right now the frontend also gets the token data we could potentially use for that (from the Meter Token table)

There are two questions here. Let me address them individually:

What should be the value for paid_for? App\Model\Token vs token

If I understand correctly paid_for defines a Polymorphic relation ship. I would like to follow the practise that is standard in Laravel. On Database level it I think it should be the class type App\Model\Token as per Laravel convention of defining polymorphic relations (but I'm also not a Laravel expert, maybe I got this wrong), rather than the display name token.

So, I'd suggest we adapt the frontend filter to check for App\Model\Token.

How should token be displayed?

My preferred solution would be to adapt the Frontend to display Token objects like this:

Token (1234-1234-1234) or even Token (123 412 341 234) (this would also resolve #83)

But this should be a pure frontend change. I.e. it should change the tokens in the backend, but just the way they are presented to the user

WDYT?

After reading up on how Laravel handles Polymorphic relationship, I now understand your explanations.

I will revert this and make the frontend change as suggested. Thank you @dmohns

dmohns · 2024-12-09T13:09:23Z

src/backend/app/Console/Commands/DemoDataCreator.php

+            // generate meter_token
+            $meterTokenData = [
+                'meter_id' => $randomMeter->id,
+                'token' => Str::random(30),
+                'energy' => round(
+                    $transactionData->transaction->amount /
+                    $randomMeter['tariff']['price'],
+                    2
+                ),
+                'transaction_id' => $transaction->id,
+            ];
+            $meterToken = $this->meterToken->newQuery()->make(['meter_id' => $meterTokenData['meter_id'],
+                'token' => $meterTokenData['token'], '' => $meterTokenData['energy'],
+                'transaction_id' => $meterTokenData['transaction_id']]);
+            $meterToken->save();


Did you manage to understand what is the difference between meterToken and the token from above? It seems very confusing to me. Looks like a place where we could refactor things, to make them more intuitive.

My understanding is that they both keep track of tokens associated with meter through the transactions table.

But there are some differences, Meter Token is a much better association because it stores a reference to the meter_id (from Meter table)

Token doesn't need to be for a meter because it has a loose model to the meter table but still is tied to a "transaction"

Basically, I'm looking at this change: c85f9aa#diff-588c01299a5ad9020a72f27600dc3dfd9cc8c4fbd8afaeb2917d0e9090f71d6d

What I'm wondering is: Is MeterToken deprecated? I.e. was MeterToken replaced by the more generic Token model?

That sounds more plausible since the frontend does expect to get tokens from transactions.

Alright, I will go with this assumption.

src/backend/app/Console/Commands/DemoDataCreator.php

beesaferoot · 2024-12-11T09:46:05Z

@dmohns Updated, kindly review.

dmohns

Few small comments, other than that LGTM

src/frontend/src/modules/Meter/Transactions.vue

dmohns · 2024-12-11T15:37:00Z

src/frontend/src/modules/Meter/Transactions.vue

+    formatToken(token) {
+      // Ensure token is a string
+      const tokenStr = String(token)
+      // Format in the desired pattern
+      // return tokenStr.match(/.{1,4}/g).join('-'); // For "1234-1234-1234"
+      return tokenStr.match(/.{1,3}/g).join(" ") // For "123 412 341 234"
+    },


Maybe you can move this into a JS mixing, similar to currency and timing where formatting functions like readable (for energy) and timeForTimeZone (for timing) are defined?

beesaferoot · 2024-12-11T18:26:50Z

Made some updates; please have a look @dmohns

dmohns · 2024-12-12T07:53:34Z

src/backend/app/Helpers/TokenGenerator.php

+class TokenGenerator {
+    /**
+     * Generate a random 12-digit token.
+     *
+     * @return string
+     */
+    public static function generate() {
+        $token = '';
+        for ($i = 0; $i < 12; ++$i) {
+            $token .= random_int(0, 9);
+        }
+
+        return $token;
+    }
+}


We'll leave this for now, but one comment for a later improvement: This should probably be a Factory https://laravel.com/docs/11.x/eloquent-factories

dmohns

Thanks 🙏

dmohns · 2024-12-12T07:56:31Z

One more non-code related comment: Please use Pull Request keywords to link PR's to corresponding issues.

For example in this PR (in the description) you can do

Closes: #396
Closes: #83

beesaferoot · 2024-12-12T08:02:11Z

@dmohns Noted your feedbacks with thanks 🙏

beesaferoot requested a review from dmohns December 9, 2024 07:45

dmohns reviewed Dec 9, 2024

View reviewed changes

src/backend/app/Console/Commands/DemoDataCreator.php Outdated Show resolved Hide resolved

beesaferoot force-pushed the meter-tokens branch from 0058b0f to 1dee06c Compare December 11, 2024 09:42

beesaferoot force-pushed the meter-tokens branch from fbc0034 to b619889 Compare December 11, 2024 13:14

dmohns reviewed Dec 11, 2024

View reviewed changes

beesaferoot added 4 commits December 11, 2024 19:23

generate random test token

efeb9e8

format token display

2ee3c67

fix prettier lint warning

e088d44

refactor token formatting using mixin

8c8af96

beesaferoot force-pushed the meter-tokens branch from b619889 to 8c8af96 Compare December 11, 2024 18:23

dmohns reviewed Dec 12, 2024

View reviewed changes

dmohns approved these changes Dec 12, 2024

View reviewed changes

dmohns merged commit 74681ba into EnAccess:main Dec 12, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use correct paid_for_type on payment #396 #400

use correct paid_for_type on payment #396 #400

beesaferoot commented Dec 8, 2024 •

edited by dmohns

Loading

dmohns Dec 9, 2024

beesaferoot Dec 9, 2024 •

edited

Loading

beesaferoot Dec 9, 2024

dmohns Dec 10, 2024

beesaferoot Dec 11, 2024

dmohns Dec 9, 2024 •

edited

Loading

beesaferoot Dec 9, 2024 •

edited

Loading

dmohns Dec 10, 2024

beesaferoot Dec 10, 2024

beesaferoot commented Dec 11, 2024

dmohns left a comment

dmohns Dec 11, 2024

beesaferoot commented Dec 11, 2024 •

edited

Loading

dmohns Dec 12, 2024

dmohns left a comment

dmohns commented Dec 12, 2024

beesaferoot commented Dec 12, 2024

use correct paid_for_type on payment #396 #400

use correct paid_for_type on payment #396 #400

Conversation

beesaferoot commented Dec 8, 2024 • edited by dmohns Loading

Brief summary of the change made

Are there any other side effects of this change that we should be aware of?

Describe how you tested your changes?

Pull Request checklist

Choose a reason for hiding this comment

beesaferoot Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmohns Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

beesaferoot Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beesaferoot commented Dec 11, 2024

dmohns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beesaferoot commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

dmohns left a comment

Choose a reason for hiding this comment

dmohns commented Dec 12, 2024

beesaferoot commented Dec 12, 2024

beesaferoot commented Dec 8, 2024 •

edited by dmohns

Loading

beesaferoot Dec 9, 2024 •

edited

Loading

dmohns Dec 9, 2024 •

edited

Loading

beesaferoot Dec 9, 2024 •

edited

Loading

beesaferoot commented Dec 11, 2024 •

edited

Loading