add charge period to transactions
AbandonedPublic
Actions

Authored by bohlender on Jan 21 2021, 9:30 AM.

Project Tags

None

Referenced Files

	F117494955: D2137.1774847300.diff
	Sun, Mar 29, 5:08 AM

Subscribers

Details

Reviewers

machniak

Group Reviewers

Restricted Project

Repository

rK kolab

Branch

transactionlog

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 32809
Build 12718: arc lint + arc unit

bohlender requested review of this revision.Jan 21 2021, 9:30 AM

bohlender created this revision.

bohlender added a reviewer: Restricted Project.Jan 21 2021, 10:01 AM

machniak requested changes to this revision.Feb 1 2021, 11:34 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
18	We're not using dateTimeTz() anywhere, we're using timestamp(). We're not using capital letters in columns names, so probably not here either. I don't like "timespan", how about `period_start` and `period_end`?
30	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, 11:34 AM

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?

bohlender marked 3 inline comments as done.Mar 30 2021, 11:44 AM

bohlender marked 2 inline comments as done.Mar 30 2021, 12:00 PM

machniak requested changes to this revision.Apr 6 2021, 7: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
18	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`.