Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add API v1 finance_overview endpoint #835

Merged
merged 1 commit into from
Feb 24, 2021
Merged

Add API v1 finance_overview endpoint #835

merged 1 commit into from
Feb 24, 2021

Conversation

paroga
Copy link
Member

@paroga paroga commented Feb 18, 2021

No description provided.

config/routes.rb Outdated
@@ -265,6 +265,7 @@

namespace :user do
root to: 'users#show'
get :finance_overview, to: 'users#finance_overview'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps "financial_overview" is cleaner here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

@@ -6,4 +6,22 @@ def show
render json: current_user
end

def finance_overview
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on scope user:read vs. finance:user?
Since this really is a financial overview, I would slightly favour the latter, with user:read more for user details and membership. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have no clear thought about our scopes yet. i'll go with your proposal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
In that case I would propose an ordergroup controller in the API that includes this method.

}
}
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you've thought about whether to do this with ActiveModelSerializer.

Other question: where will we expose Ordergroup information? We don't really, right now, and it is perhaps related to this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't see a benefit of a serializer in this case. i'd refactor, if we decide that there's a better way

do you you mean with Ordergroup information? we expose our ordergroup-related finacial_transactions already via /user/financial_transactions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. The API doesn't currently return e.g. the name of the user's ordergroup, or whether it has one.
If we were to add that, would there be overlap with financial_overview?

One way would be to let /api/user also return group memberships, and have /api/ordergroup show ordergroup fields. Another way would be to only have /api/user/ordergroup with ordergroup fields (and then perhaps /api/user/workgroups for workgroup memberships). The user's ordergroup endpoint would either include the financial overview, or there would be a separate /api/user/financial_overview endpoint.

Since any choice we make will stick for quite a while, I'd like to put some thought to it.
What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRK the basic idea of /api/user was to make dealing with access roles easier: every user can request /api/user/financial_transactions to get her own transactions, but only finance-people can request /api/financial_transactions to get all transactions.
sticking to that idea /api/user/finance_overview should be the way to go. I see no benefit in moving it to /api/user/ordergroup/finance_overview, since a user has only one and we would need to move the /api/user/financial_transactions to /api/user/ordergroup/financial_transactions then too. IMHO renaming /api/user to /api/me or /api/my would make the basic idea more clear, but we settled with /api/user already.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRK the basic idea of /api/user was to make dealing with access roles easier

True.

Sticking to that idea /api/user/finance_overview should be the way to go

Yes.
What I mean is that we can choose between including the financial overview in the /api/user/ordergroup endpoint, or put it in /api/user/financial_overview. I'd be ok with both. My main motivation for bringing this up is to ensure that we won't have a confusing situation when a user ordergroup endpoint is introduced.

Perhaps the account balance and available funds may be present in both the /api/user/ordergroup and /api/user/financial_overview endpoints, but that doesn't hurt so much. Looking at the scope definitions, we could include /api/user/ordergroup in the user:read scope and leave /api/user/financial_overview with the finance:user scope. That would point to not including account balance and available funds in the /api/user/ordergroup endpoint.

Concluding: the current /api/user/financial_overview endpoint approach seems ok to me.

Then the user controller seems a sensible place for the /api/user and /api/user/ordergroup endpoints. A future /api/user/workgroups endpoint would probably warrant its own controller. And the /api/user/financial_overview could either live in the user controller (with proper scope checking), or in its own financial-overview-controller. Both seem ok.

... /api/me or /api/my ...

Agree that /api/me may have been clearer, indeed that's past a decision point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what you've done now seems even better, on second thought: putting it in an ordergroup controller. 👍

@paroga paroga merged commit 7a6779e into foodcoops:master Feb 24, 2021
@paroga paroga deleted the fo branch February 24, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants