Page MenuHomePhorge

add charge period to transactions
AbandonedPublic

Authored by bohlender on Jan 21 2021, 10:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 24, 3:15 PM
Unknown Object (File)
Sun, Mar 24, 2:30 PM
Unknown Object (File)
Wed, Mar 20, 1:43 AM
Unknown Object (File)
Tue, Mar 19, 4:35 PM
Unknown Object (File)
Tue, Mar 19, 1:33 AM
Unknown Object (File)
Mon, Mar 18, 1:22 AM
Unknown Object (File)
Sun, Mar 17, 1:25 PM
Unknown Object (File)
Sat, Mar 16, 6:55 AM
Subscribers

Details

Reviewers
machniak
Group Reviewers
Restricted Project

Diff Detail

Repository
rK kolab
Branch
arcpatch-D2137_1
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 33754
Build 13363: arc lint + arc unit

Event Timeline

bohlender created this revision.
bohlender added a reviewer: Restricted Project.Jan 21 2021, 11:01 AM
machniak subscribed.
machniak added inline comments.
src/app/Entitlement.php
84

Missing space.

src/app/Transaction.php
49

Missing space.

src/app/Wallet.php
98

Removed line that should not be removed.

src/database/migrations/2021_01_14_093842_add_timespan_to_transactions_table.php
19
  1. We're not using dateTimeTz() anywhere, we're using timestamp().
  2. We're not using capital letters in columns names, so probably not here either.
  3. I don't like "timespan", how about period_start and period_end?
31

Missing drop column statements.

src/resources/vue/Widgets/TransactionLog.vue
114

Missing space.

115

I think we should use Laravel's type casting https://laravel.com/docs/6.x/eloquent-mutators#date-casting

This revision now requires changes to proceed.Feb 1 2021, 12:34 PM

And of course some tests will be needed.

src/resources/vue/Widgets/TransactionLog.vue
118

What if this is the same day? Should we display that differently? Should we display additional "X days" text?

  • rename timespanStart/timespanEnd to period_start/period_end
machniak requested changes to this revision.Apr 6 2021, 9:16 AM
machniak added inline comments.
src/app/Entitlement.php
81

Missing documentation for the added arguments.

src/app/Transaction.php
19

Please, document 'period_start' and 'period_end' properties here.

src/database/migrations/2021_01_14_093842_add_timespan_to_transactions_table.php
19

Not done. The column names are wrong. Also, what happens with the existing records? I think it should be nullable() instead of useCurrent().

src/resources/vue/Widgets/TransactionLog.vue
114

Not done. Missing space after if.

This revision now requires changes to proceed.Apr 6 2021, 9:16 AM