Skip to content
This repository has been archived by the owner on Oct 3, 2019. It is now read-only.

Recurring transaction. byDate, byDateRange, and how returned in month. #52

Open
henriean opened this issue Jul 22, 2019 · 11 comments
Open
Assignees
Labels
question Further information is requested
Milestone

Comments

@henriean
Copy link
Contributor

First:
What should be returned for byDate and byDateRange, and what do we need?

Should recurring/byDateRange just return the recurring transaction objects which have at least one occurrence within that range? Or should a list of all date occurrences, within that date range, be given with the recurring transaction (as in get month)?

For recurring/byDate, should a recurring transaction be returned if it has a recurring transaction on that day, or just if it is still active that day?

Second:
From the way month is implemented there is another issue regarding how recurring transaction occurrences are returned. As by now you'll have a list of dates and a list of transaction-ids for the occurrences that were changed and needed to be saved as its own transaction. The list of dates, however, return the dates of changed ones as well.
Also, all transactions of the month are returned (thus including the ones coming from recurrent transactions). This means that front end needs to figure out which dates it should not display (or it will be doubled up).

One option would be to see if a transaction has a recurrent_id (meaning it is a changed recurrent transaction), and then delete its date from this recurrent transactions list.
This is a bit stress for the front-end, plus I see one problem with this:
There is nothing stopping a user to change a recurring transaction occurrence to the same day as another recurring occurrence. If there are two on the same day for the same recurring transaction, then this method will remove the date where there should actually be added one generic occurrence, thus skipping this one.

I believe the easiest way would be to include a list of dates for just the generic recurring transactions that follows the template, and let the altered ones either just be returned in the list of transaction_ids that the recurring transaction holds, or together with the normally stored transactions.

This leads up to the last point:
Is the API too complex when it comes to transactions? I agree that recurrent transactions should be saved as templates (avoiding saving too much redundant data), but should the way it is returned be changed?

@henriean henriean added the question Further information is requested label Jul 22, 2019
@henriean henriean added this to the API 1.0.0 milestone Jul 22, 2019
@nicomni
Copy link

nicomni commented Jul 23, 2019

Can you please add example JSON schemes to illustrate your points for readability?

@nicomni
Copy link

nicomni commented Jul 23, 2019

I think it's important to resolve this discussion. As this discussion seems a bit complicated I believe it's more effective if we discuss this face-to-face in a group meeting.

@nicomni
Copy link

nicomni commented Jul 23, 2019

❓ I would like to question the validity of these endpoints.

What requirements do they exist to fulfill?
Can someone describe a use case/user story where these endpoints would be used?

(Just trying to adapt a lean philosophy here 😊 )

@kalkins
Copy link
Collaborator

kalkins commented Jul 24, 2019

@henriean
I think that byDate and byDateRange should return recurring transactions that have occurences within that range, and return on the same format as the recurring field of month.

@NicolajNiels1
The API wasn't designed with user stories in mind, and only with a vague understanding of what the frontend would need, so I added endpoints that could be useful, not that I knew would be used.

byDate is probably not needed. byDateRange could be useful if the user should be able to manage only recurring transactions, which probably need to be edited more than transactions that are explicitly added. However, I don't know if this is a goal, and it could probably be done with the month API anyway

@kalkins
Copy link
Collaborator

kalkins commented Jul 24, 2019

@henriean
Regarding the Month API I agree that we have to change something about how recurring transactions are returned. I think the best approach is to return the dates as an object containing the date and a field field saying if the transaction has been explicitly defined or not (this could also be the id of the transaction). The alternative is to remove those dates from the array

I think it shouldn't be possible to save an occurence on a date on the date of another occurence. The backend should have a check against this. The frontend might handle it by saving as the second occurence instead.

@kalkins
Copy link
Collaborator

kalkins commented Jul 24, 2019

Another issue is what to do if an occurence doesn't happen. Should we just set money to 0, add a field saying it's deleted, or something else?

@nicomni
Copy link

nicomni commented Jul 25, 2019

Another issue is what to do if an occurence doesn't happen. Should we just set money to 0, add a field saying it's deleted, or something else?

Yes. Like of the user wants to skip a payment or income statement one month (for monthly recurrence). Some way of flagging this would be ideal! 🤔

Sent with GitHawk

@FredrikAugust
Copy link
Contributor

Adding another field would require more data to be sent, and changes to interfaces.

I think setting the money field to zero should suffice, as there's no reason to show a transaction that does not affect the liquidity.

This would also require minimal changes to the frontend, as you could just add this when reading from the transaction store;

.filter(e => e.money !== 0)

@kalkins
Copy link
Collaborator

kalkins commented Aug 2, 2019

@FredrikAugust @NicolajNiels1 @KaizerTilt
I think the best approach would be to change the dates list to include objects containing the date and a flag saying whether the transaction has been explicitly saved or deleted:

{
    date: "2019-07-20",
    type: SAVED | NOT_SAVED | DELETED
}

I'm open for suggestions on the names.

@FredrikAugust
Copy link
Contributor

FredrikAugust commented Aug 5, 2019

I think your proposal is sane, and would make the frontend implementation a whole lot easier.

As for the naming, I think date is fine, and if you would like me to be nitpicky; I believe state might be better for the type field.

@FredrikAugust
Copy link
Contributor

byDate and byDateRange should return all recurring transactions that are relevant in that time period IMO. So, if the day is between start_ and end_date, it should be included.

FredrikAugust added a commit to Kodeworks/budsjetteringssystem that referenced this issue Aug 5, 2019
Skipped testing /byDate and /byDateRange for recurring due to its
implementation details being under discussion. Ref. Kodeworks/liquidator-backend#52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants