Page MenuHomekolab.org

add charge period to transactions
Needs RevisionPublic

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

Details

Reviewers
machniak
Group Reviewers
Restricted Project

Diff Detail

Repository
rK kolab
Branch
arcpatch-D2137_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 33757
Build 13366: arc lint + arc unit

Event Timeline

bohlender requested review of this revision.Jan 21 2021, 10:30 AM
bohlender created this revision.
bohlender added a reviewer: Restricted Project.Jan 21 2021, 11:01 AM
machniak requested changes to this revision.Feb 1 2021, 12:34 PM
machniak added a subscriber: machniak.
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?

bohlender updated this revision to Diff 6763.Mar 30 2021, 1:43 PM
  • add missing spaces
bohlender marked 3 inline comments as done.Mar 30 2021, 1:44 PM
bohlender updated this revision to Diff 6769.Mar 30 2021, 1:52 PM
  • rename timespanStart/timespanEnd to period_start/period_end
bohlender updated this revision to Diff 6775.Mar 30 2021, 1:59 PM
  • use timestamp instead
bohlender marked 2 inline comments as done.Mar 30 2021, 2:00 PM
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