-
Notifications
You must be signed in to change notification settings - Fork 147
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
API-3: financial transactions #572
Conversation
1bfeee6
to
9f700bd
Compare
9f700bd
to
406ade0
Compare
406ade0
to
79021e7
Compare
tags: | ||
- 6. FinancialTransaction | ||
parameters: | ||
- $ref: '#/parameters/page' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we combine this two? they are use together more or less ever IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be possible.
Don't you think it is clearer to list all parameters here, instead of having to lookup the definition to see what parameter names they contain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's different to Meta
? but i can live with both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta
is something that is returned, these parts are all described in models below. parameters
are what you supply as part of the request.
I've tried various ways of structuring parametes in other API definitions, and settled on this flat list, which turned out to be the easiest to comprehend: it just lists the parameters, so you don't need to go look downward to find which parameters are accepted when reading this file.
private | ||
|
||
def scope | ||
current_ordergroup.financial_transactions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for seeing that so late, but shouldn't we scope to the current ordergroup via ransack? how is an admin later able to see all transaction? having and second endpoint for that seams ugly to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API v1 is explicitely designed to be for users, not admins - I didn't want to have to design the whole admin API before I could start. If we want to do so, I suggest to target API v2.
Besides, experimenting with the API, as well as writing API clients that don't do any admin stuff, would become more complicated, because one would always need to include your own ordergroup id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why can't it be backward compatible? i could live with adding a /my/financial_links
endpoint now, which allows us to a more general later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you want to search through all transactions, for example? Same for other endpoints - I'd definitely want to search among all users, so we'd need some users
endpoint that isn't scoped. I haven't taken the time to work this out, and wouldn't really want to do so before finishing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of my firsts steps withe the new API will be to work with the financial transactions (across users), having to replace the API version so soon doesn't seam intelligent to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about "hardcoding" some parameters for now? e.g something like raise 403 if param[:ordergroup] != current_ordergroup
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experimenting with the API, as well as writing API clients that don't do any admin stuff, would become more complicated, because one would always need to include your own ordergroup id.
I'd like to take time to think about URL structure for the full API before doing so.
Would making the current API v0 be an ok compromise?
But let me think aloud to see if we get anywhere.
/api/v1/user
->/api/v1/users/me
/api/v1/navigation
-> fine [though perhaps multiple menus would be useful]/api/v1/config
-> fine [and can easily add update methods]/api/v1/article_categories
-> fine [can add update]/api/v1/orders
-> could be ok, though perhaps admins want to see orders in the future if we automate opening orders. Client could handle that, but I'd consider that a drawback, as not all client developers may try all these corner-cases both as user and as admin with different access levels./api/v1/order_articles
-> currently makes sense for just open orders, but just querying all order articles doesn't; do we scope this for (single, multiple) orders then? Or do we just allow dumping all order articles in the database, and filtering will in practice always be used? What would we do with the same order article happening in multiple orders, would that be confusing? Perhaps we can provide some basic scopes for this (through orders, like the orders endpoint uses). [can add update]/api/v1/group_order_articles
-> like order articles. This would generally be scoped by order, or order and ordergroup, or ordergroup and something like a timespan or so. [can add update]- members, tasks, wiki
- would we add special endpoints for things like receive and balancing, or expect to do so by updating the direct endpoints? Would that be error-prone (i.e. require a lot of care from API developers)? [not that relevant now]
This is not intended to start a discussion here to solve these issues, but just share some things that would be on my mind when designing a full API.
I'd go for adding APIs when the need arises. Foodsoft-shop would be happy with the current API, but if things like viewing older orders are added, would soon run into limitations. Still, I would not want to wait releasing foodsoft-shop until all possible additions are done.
Your need would be to access all financial transactions. That could be done by keeping /api/v1/financial_transactions
as an admin endpoint (or perhaps when you add a scope you have access to - though this could make authorization more complex) use /api/v1/ordergroups/my/financial_transactions
for the user's financial transactions. This already brings some kind of a distinction between 'admin' and 'user' endpoints, though ...
Perhaps it we should add a v0 first, and don't guarentee stability until v1?
Or add a v1 endpoint with stability for member access, and an experimental v1b or v2alpha without stability and go v2 when that is satisfactory? That way we can both keep going forward, and have some freedom of experimentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i personally don't want to push "testing code" into master. I see a big benefit in getting basic model and database changes of half finished code into master to avoid a migration difficulties when using non-master code somewhere, but I see no real benefit in adding UI and API endpoints to master, which will be subject to big change in the near future.
Is there a point, why you want it integrated in master, instead of working on it in a feature branch? I REALLY like all the work you put into it (RESPECT and BIG THX!!), but I also don't want to rush over it. I'd like to just integrate code of which we know that it's ready for v1
. So we can both have the same base to build our future work on top and do our EXPERIMENTS, but keep the official version away from migration problems.
What additional use cases for the API do you have in mind, beside switching the existing functions from server rendering to client rendering?
I know that it gets already off-topic, but you have already a migration strategy for the existing UI? I'm thinking about a hybrid as the first step, where the base layout stays like it is (but with newer bootsrap and so on), but we put some React widgets into the view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: I think making the API available helps the Foodsoft 'ecosystem' (instead of waiting like another year).
My main motivation is making significant steps with foodcoop-adam#163. I do feel some time pressure there, because the foodcoop-adam fork is unmaintained, right now. I'd like to move all its users to the global instance. But that should not really be a reason for rushing the API.
Still I think having an API - even though not covering all functionality - can really help others to put Foodsoft to use better. It is more welcome to developers familiar with other technologies than Rails, and allows people to use their customizations with stock Foodsoft (especially those using hosted Foodsoft). Having that somewhere soon, I think, is better than waiting for another year (it's a bit of a small miracle that I was able to devote substantial time to work on Foodsoft).
When it becomes a maintenance nightmare, or is likely to be a source of problems later, we'd better think twice. Having an additional API version to maintain could be a certain burden later, but as the layer is thin (and if the serializers are moved to a v1 namespace when they become really different, not in the way) I think the tradeoff is worth it.
Making a distinction between 'member'-functionality and 'admin'-functionality: for the groups I'm in contact with, member-functionality is highest on the list to provide more polished interfaces for, because admin-tasks need training already. Having an API even allows integrating the front-end with other existing member-facing systems.
Another thing I'd like to do with the API is the member financial transactions overview (relates to #398), integrating pay-after-order/shopping-cart-like, and later perhaps rewriting the current_orders plugin (which includes a point-of-sale module, which I'd like to improve in the future). And then on to some use-case-specific admin-screens, e.g. like distributing veggies using a scale (perhaps even connected to a device). Or perhaps a mobile app for checkout scanning by a workgroup and/or a member him-/herself. But that's for much later, if ever.
I have no all-encompassing migration strategy in mind. I guess it will also depend on who is stepping up to do the work. With the API there are now multiple options:
- pure Rails-based like it is now: in my experience it is harder to make interactive interfaces while keeping a clean codebase. But it is the most straightforward approach, if you're familiar with Rails.
- Javascript components served from Rails views, using session authentication with non-API endpoints: that route is open and similar to 1.
- Javascript components served from Rails views, using the API: this would use access tokens (which could be fetched using the OAuth dance, or generated from within Foodsoft directly; alternatively the Foodsoft session could be used, but I'd not favour that). It can be developed and run separately from Foodsoft.
- Mini-apps using the API: a full-fledged app like foodsoft-shop, like 3 but then with their own layout (which would be candidate to make an npm module of). These can easily be run standalone, connected to an existing Foodsoft instance (perhaps ideally we'd make component out of it, allowing both embedded and standalone usage).
My current bet would be that core member-functionality might move to a pure front-end using the API (if there really is desire for an updated UI). Admin-functionality will be mixed, and depending on the developers it would use one of the options above (though we might limit it to two or so). And see how it goes from there, that's probably a couple of years. But I suspect you'd prefer 1, probably also for member-functionality, and that's something we'll have to figure out. In my view, that is also related to how others jump in. Let's do what works for us as a community of users, developers and sysadmins.
Thanks for having this discussion :) (which perhaps we'd move to the mailing list at some point)
79021e7
to
6bc908c
Compare
I'd like to read a little bit of the subtext, so please correct me if I'm wrong. You want to get rid of the adam fork and don't want to implement it in the "old" rails fashion. Implementing and letting it rot in a branch where nobody can use it is not funny, so you want to get experience in production with your new interface. For that you need the API in the version, which get's deployed to the groups where you want to test the features in real world. I ran 2 different branches for the Austrian foodcoops a long time, where just the slug in the URL was responsible for choosing the foodsoft container by haproxy. One used the "offical" foodsoft version and the other one has some additional features implemented, which where only accessible to a limited set of groups. Can you imagine something like that for the remaining adam-foodcoops?
Yes, but this also makes sense to my if we can keep the API clean and maintainable.
IMHO that's irrelevant if we have "problematic" constraints like "current orderdergrup".
i completely agree, but i'd like to have a plan how we want to proceed with the admin stuff. i don't want to end up with two foodsoft versions. one shiny polished new version for the member stuff and an old ugly admin version. And then, since the API is not compatible, nobody will do the work of adopting the admin interface, while it gets older and older. option 1+2 is very outdated and i see no bright future for them, but since we have no better option (yet) i use it for new features. option 3 is for me only a migration strategy to 4. i'd like to see Foodsoft becomming a full-fledged client application, which uses only the API.
Do you see anybody jumping in ATM? PS: Once again sorry for being so picky about this, but IMHO this is a big decision for future of foodsoft and i don't want to rush over it. |
Good you're asking pointy questions :) I'll start by answering the 'subtext'.
Almost, but not quite. The old approach, using rails views, was an experiment, something to see if it would work. My lesson was that doing this kind of interactive stuff (where things in the UI update at different places right away) becomes a mess when trying to do that in a rails+jquery-based app, with code duplication and many cases where things can go (sublty and bluntly) wrong. Do do this properly, I learned and still believe, is to use an API, with a clear behaviour, and let the front-end do the interactiveness, without tangling up with the rails-side of things. And indeed, when using an API, code turned out to be much clearer and testable, and has the benefit of being fit for other uses as well. This is a 'clean' way of doing the things I tried to do before. In my book, it's not abandoning old code, it's taking lessons learned and start from scratch to build something that may last. And to avoid this being a mega-project taking years (which it ended up being anyway), I narrowed the scope of the API to member-parts only. Small incremental bits, that's how open source works best, right? Besides, foodcoops' Foodsoft went on, sometimes in a different direction that foodcoop-adam, and it become quite clear that some things had to be re-made or be lost forever. I contributed back some parts that did fit in (some visible by users, others only in the code structure so that plugins could more easily hook things), others needed a new implementation. Like member ordering and being able to enter amounts in weight/volume units instead of pieces. When foodsoft-shop and the API got mostly feature parity with Foodsoft, I did some of my own testing, and another Foodsoft user performed a test with it in his foodcoop (out of the blue for me), which was successful. So I conclude that it is ready enough to be integrated. I hope this story makes visible that my approach is not just something to do a quick fix, but a complete rewrite to fix a broken system, and make something that can last. Even when it is just a small step (from the outside, it took 2y+).
That could work as well, it would basically be this PR plus perhaps some other things in the future.
Perhaps #429 (comment). I have also worked with people who wanted to be involved, but got stuck on diving into Foodsoft. An API could have helped there.
I kind of hear you saying that you don't want to see the admin parts staying behind, that it's really important to allow them to flourish. |
Ok, I've taken sime time to think about an admin API - #582 (comment) |
100% agree, but we should define the direction first. 😉
No worry. I see the work, which went into it! It's a HUGE step forward.
not really, but I don't want to introduce completely new technology and most of my stuff was simple CRUD
YES, i want to have a path to switch everything to v1 (or at least be able to do so), even if it will take some while, but this is IMHO the only way to get some kind of foodsoft app working, which can also e.g. create new invoices from a photo taken directly via the app. |
I'm reworking this PR to view both the user's financial transactions, as well as everyone's (see also #582 (comment)). That could show a path forward to a complete API. |
the basic idea LGTM, but it's very hard to make a good review with this big change. 😞 could you split it up into smaller logical chunks with a clear history? so we could merge it also incrementally do we stick with our current roles and just adopt it as scopes, or do we want to make it more fine grain? (i'm ok with the current roles) |
Part three of #429 in chunks, continued from #571:
/api/v1/financial_transactions
/api/v1/financial_transactions/:id
/api/v1/user/financial_transactions
/api/v1/user/financial_transactions/:id
This doesn't include transaction types or classes yet, since I haven't really worked out how to work with them in an application. Will they have separate endpoints, or is relevant data in the financial transactions endpoint enough? To not over-engineer it, I'd like to postpone adding that until the need arises.
See also the Swagger UI.