kredits/kredits-web
9
0
Fork 0
Code Issues 22 Pull Requests 1 Actions Releases 18 Activity

Introduce budget, reimbursements for expenses #195

Merged
raucao merged 57 commits from feature/expenses into master 2021-06-03 14:23:45 +00:00
Conversation 11 Commits 57 Files Changed 76
+3322 -765
raucao commented 2020-10-01 10:56:18 +00:00 (Migrated from github.com)
Copy Link
Copy Source

This is a big one. It isn't linked from the dashboard yet, but you can browse to /budget and propose reimbursements from there.

Requires https://github.com/67P/kredits-contracts/pull/198
Refs 67P/kredits-contracts#48

To do

  • Fix ember linting errors
  • Sync new and missing reimbursements from network when cached data loaded
  • Fix cache loading after adding new reimbursements (records shows up twice)
  • Fix bug, which claims expense amounts are missing, after tx creation
This is a big one. It isn't linked from the dashboard yet, but you can browse to `/budget` and propose reimbursements from there. Requires https://github.com/67P/kredits-contracts/pull/198 Refs 67P/kredits-contracts#48 ## To do * [x] Fix ember linting errors * [x] Sync new and missing reimbursements from network when cached data loaded * [x] Fix cache loading after adding new reimbursements (records shows up twice) * [x] Fix bug, which claims expense amounts are missing, after tx creation
bumi (Migrated from github.com) reviewed 2020-10-01 10:56:18 +00:00
raucao commented 2020-10-30 14:30:59 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I just finished the functionality of the PR, so there are just the linter errors left. You can test it on your machines now.

There's one known issue, which won't be much of a problem in a production: a race condition, when your tx confirms within the loading of the reimbursement list after pressing the submit button on the reimbursement form. I'm not sure how to handle that, and it doesn't break anything in the unlikely case that you see this in production. Just reload the page and all is good, due to the caching and syncing logic.

I would recommend to always add a block time to your local devchain when testing UI anyway, to not get the wrong feel for how things work in production: e.g. npm run devchain -- --block-time 5.

I just finished the functionality of the PR, so there are just the linter errors left. You can test it on your machines now. There's one known issue, which won't be much of a problem in a production: a race condition, when your tx confirms within the loading of the reimbursement list after pressing the submit button on the reimbursement form. I'm not sure how to handle that, and it doesn't break anything in the unlikely case that you see this in production. Just reload the page and all is good, due to the caching and syncing logic. I would recommend to always add a block time to your local devchain when testing UI anyway, to not get the wrong feel for how things work in production: e.g. `npm run devchain -- --block-time 5`.
raucao commented 2020-10-31 12:43:01 +00:00 (Migrated from github.com)
Copy Link
Copy Source

All done and ready to merge from my point of view.

All done and ready to merge from my point of view.
galfert (Migrated from github.com) requested changes 2020-11-17 11:13:53 +00:00
galfert (Migrated from github.com) left a comment
Copy Link
Copy Source

Wow, that is a lot of changes :)

Left a few comments.

So far I wasn't able to test it manually, because of problems running the contracts (see XMPP).

Wow, that is a lot of changes :) Left a few comments. So far I wasn't able to test it manually, because of problems running the contracts (see XMPP).
app/components/add-expense-item/component.js
@@ -0,0 +1,86 @@
import Component from '@glimmer/component';
galfert (Migrated from github.com) commented 2020-11-13 14:49:01 +00:00
Copy Link
Copy Source

Is this intended? If it just returns true, why is it needed?

Is this intended? If it just returns true, why is it needed?
app/components/add-reimbursement/component.js
@@ -0,0 +127,4 @@
title: `Expenses covered by ${contributor.name}`,
description: this.description,
url: this.url,
expenses: JSON.parse(JSON.stringify((this.expenses)))
galfert (Migrated from github.com) commented 2020-11-13 15:20:34 +00:00
Copy Link
Copy Source

What's the reason for this stringify/parse combo? Probably a good idea to add a code comment for this.

What's the reason for this `stringify`/`parse` combo? Probably a good idea to add a code comment for this.
app/components/topbar-account-panel/component.js
@@ -34,7 +34,6 @@ export default Component.extend({
this.set('setupInProgress', true);
galfert (Migrated from github.com) commented 2020-11-13 15:45:31 +00:00
Copy Link
Copy Source
```suggestion ```
app/services/community-funds.js
@@ -0,0 +16,4 @@
yield fetch(uri).then(res => res.json())
.then(res => this.processBalances(res))
.catch(err => {
galfert (Migrated from github.com) commented 2020-11-13 16:02:52 +00:00
Copy Link
Copy Source

fetch doesn't reject the promise on normal HTTP errors (i.e. 400 or 500 errors), but only on network failures. So this should probably also check if res.ok is true.

`fetch` doesn't reject the promise on normal HTTP errors (i.e. 400 or 500 errors), but only on network failures. So this should probably also check if `res.ok` is `true`.
app/services/exchange-rates.js
@@ -0,0 +9,4 @@
try {
const res = await fetch(`${bitstampBaseUrl}/ticker/${currencyPair}/`).then(r => r.json());
return res.vwap; // Last 24 hours volume weighted average price
} catch(e) {
galfert (Migrated from github.com) commented 2020-11-13 16:05:44 +00:00
Copy Link
Copy Source

Same as the comment in the community-funds service. This should probably check if the response is ok.

Same as the comment in the `community-funds` service. This should probably check if the response is ok.
app/services/kredits.js
@@ -406,23 +447,185 @@ export default Service.extend({
});
galfert (Migrated from github.com) commented 2020-11-16 12:49:13 +00:00
Copy Link
Copy Source

Returning a promise here is redundant, because that's the default for async functions.

Returning a promise here is redundant, because that's the default for async functions. ```suggestion ```
raucao (Migrated from github.com) reviewed 2020-11-17 11:47:14 +00:00
app/components/add-reimbursement/component.js
@@ -0,0 +127,4 @@
title: `Expenses covered by ${contributor.name}`,
description: this.description,
url: this.url,
expenses: JSON.parse(JSON.stringify((this.expenses)))
raucao (Migrated from github.com) commented 2020-11-17 11:47:13 +00:00
Copy Link
Copy Source

It's to get rid of the EmberObject properties and only retain the pure expense data in the object handed to the wrapper.

It's to get rid of the EmberObject properties and only retain the pure expense data in the object handed to the wrapper.
raucao (Migrated from github.com) reviewed 2020-11-17 11:49:20 +00:00
app/services/exchange-rates.js
@@ -0,0 +9,4 @@
try {
const res = await fetch(`${bitstampBaseUrl}/ticker/${currencyPair}/`).then(r => r.json());
return res.vwap; // Last 24 hours volume weighted average price
} catch(e) {
raucao (Migrated from github.com) commented 2020-11-17 11:49:20 +00:00
Copy Link
Copy Source

If it is not OK, then this already just gracefully fails by not automatically converting numbers.

If it is not OK, then this already just gracefully fails by not automatically converting numbers.
raucao (Migrated from github.com) reviewed 2020-11-17 12:37:45 +00:00
app/services/community-funds.js
@@ -0,0 +16,4 @@
yield fetch(uri).then(res => res.json())
.then(res => this.processBalances(res))
.catch(err => {
raucao (Migrated from github.com) commented 2020-11-17 12:37:45 +00:00
Copy Link
Copy Source

This gracefully fails by catching the exception in processBalances I think.

This gracefully fails by catching the exception in `processBalances` I think.
raucao (Migrated from github.com) reviewed 2020-11-17 12:38:26 +00:00
app/services/kredits.js
@@ -406,23 +447,185 @@ export default Service.extend({
});
raucao (Migrated from github.com) commented 2020-11-17 12:38:26 +00:00
Copy Link
Copy Source

Didn't know that. Not sure I like it being implicit here...

Didn't know that. Not sure I like it being implicit here...
raucao (Migrated from github.com) reviewed 2020-11-17 12:39:47 +00:00
app/components/add-expense-item/component.js
@@ -0,0 +1,86 @@
import Component from '@glimmer/component';
raucao (Migrated from github.com) commented 2020-11-17 12:39:47 +00:00
Copy Link
Copy Source

I guess it's just a leftover from before the proper form validation.

I guess it's just a leftover from before the proper form validation.
raucao commented 2021-06-02 12:27:33 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@galfert Did you see my replies? I just merged the corresponding contracts PR, so it'd be great if we can merge this one as well now.

@galfert Did you see my replies? I just merged the corresponding contracts PR, so it'd be great if we can merge this one as well now.
galfert (Migrated from github.com) approved these changes 2021-06-03 10:19:26 +00:00
galfert (Migrated from github.com) left a comment
Copy Link
Copy Source

LGTM

LGTM
Sign in to join this conversation.
Reviewers
No Reviewers
bumi
raucao
galfert
Labels
Clear labels
bug
Something is not working
 dev environment
Config, builds, CI, deployment, etc.
 docs
Documentation
 duplicate
This issue or pull request already exists
 enhancement
Improving existing functionality
 feature
New functionality
 idea
Something to consider
 invalid
Not a bug
 kredits-1
Small contribution
 kredits-2
Medium contribution
 kredits-3
Large contribution
 question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
 ui/ux
User interface, process design, etc.
 wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/kredits-web#195
Delete Branch "feature/expenses"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?