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

feat: add loan category doctype #19

Closed
wants to merge 3 commits into from
Closed

feat: add loan category doctype #19

wants to merge 3 commits into from

Conversation

anandbaburajan
Copy link
Contributor

  • Added a Loan Category doctype, which would be used to group different Loan Types together.
  • Added a Loan Category field in Loan Type and in Loan (just for info purposes)

@bosue
Copy link
Contributor

bosue commented Sep 14, 2023

I really don’t want to offend anyone, but at this point I’m unsure if at this point community interaction or collaboration in this rather new project is even wanted or if maintainers have a concrete idea where they want to go with this project and do their thing anyway…?

Either way it‘s completely unclear what this PR is supposed to meaningfully do. An (otherwise empty) bag of loan types?

Wherever you may look, “Loan types” or types of loans are already the top category, including:

Sure, you may have “categories” of unsecured and of secured loans, of term loans and revolving loans, of fixed-rate loans and variable-rate loans, of consumer and business loans, and then a particular loan type like an unsecured, fixed-rate, B2B term loan would fall into three or four of those “categories”.

But I don’t think that’d make all too much sense at this point.

Rather I would propose following through and building upon my suggestions in #15 to only keep the most stable, most consistent aspects such as „Is Term Loan“ in the Loan Type DocType plus some more parameters that will be common to most products of this loan type, such as the corresponding CoA accounts. And then you’d add some more very basic aspects that really define a particular loan type.

The remaining details may be moved down a level, to a Loan Product DocType. A top category above loan types – essentially a bag of just a handful of loan types – however doesn‘t seem to be all too helpful.

To some extent this may be a naming issue. We could rename Loan Types to Loan Products, Loan Categories to Loan Types and move the more general properties a level up rather than moving the more peripheral variables a level down, effectively ending up with the same result. I don‘t think that‘s the better approach though, it rather seems messy.

But again: if you already do have your battle plan and just want to put it through, I will no longer bother with discussing PRs and trying to figure out what might be a sensible way to further develop this project, until community interaction is asked for. Our own plans that – besides our commitment to support open source – are guided by business needs will then probably take a different route, which may be fine, though... 💁🏻‍♂️

Best regards and no hard feelings, please! 🤗

@anandbaburajan
Copy link
Contributor Author

First of all, thanks @bosue for your detailed thoughts and contributions!

Not concrete, but we have a good idea where we want to go since a customer is helping us with ideas and feedback on how to drive this project. But we would love to keep getting your feedback and contributions! This does seem like a naming and moving some fields up/down problem. Your points do make some sense on first read, so we'll definitely look further into it.

@anandbaburajan anandbaburajan marked this pull request as draft September 19, 2023 17:03
@bosue
Copy link
Contributor

bosue commented Sep 20, 2023

Propose closing this to instead add a Loan Product doctype a level below Loan Types.
I‘m even prepared to prepare a PR once #39 is committed. You may then see whether all you were trying to achieve can be done this way just as well.

@codecov-commenter
Copy link

Codecov Report

Merging #19 (56acc62) into develop (14ea3e3) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop      #19      +/-   ##
===========================================
- Coverage    48.00%   47.95%   -0.06%     
===========================================
  Files           51       52       +1     
  Lines         2756     2759       +3     
===========================================
  Hits          1323     1323              
- Misses        1433     1436       +3     
Files Changed Coverage
..._management/doctype/loan_category/loan_category.py 0.00%

@bosue
Copy link
Contributor

bosue commented Sep 22, 2023

If you need the category, you need the category… 🤷🏻‍♂️

If you like the idea of a Loan Product, it is already in the works. I wanted it to be really good before posting a PR, but I may also accelerate it a bit.

@anandbaburajan
Copy link
Contributor Author

@bosue we'll be merging this for now since we need something, but we're looking forward to doing the renaming and moving fields. I like the idea of loan product, and renaming loan type to loan product seems ideal to make migration simple. We can move fields up to loan category or loan product category or loan type (not sure yet about the name). Some softwares have both product category and product type.

@bosue
Copy link
Contributor

bosue commented Sep 22, 2023

@anandbaburajan: Too bad, if this is only because you‘re in a hurry. How much time could you give me to come up with a viable Loan Product doctype instead? 6 hours should be enough.

@anandbaburajan
Copy link
Contributor Author

@bosue you can take your time, we can wait until early next week 😄

@bosue
Copy link
Contributor

bosue commented Sep 22, 2023

@anandbaburajan: Re the interesting (!) link you provided me: that‘s correct, they do have categories as well, but categories are m:n there, i.e. orthogonal.

ERPNext does actually allow for orthogonal m:n links using Table MultiSelect, and if done this way I would definitely support categories. ATM, it doesn‘t look like that‘s the case here, though.

PS: Rest assured, this is not about (my) personal or (our specific) business needs, only about getting things right. ☺️

@bosue
Copy link
Contributor

bosue commented Sep 23, 2023

Please see #54 for preliminary code introducing a Loan Product below Loan Type.

Still need to figure out if and how documents can be filtered for containing particular MultiSelect values (i.e. for being tagged as x,y or z). Then this mechanism would allow us sufficient means for categorizing Loan Types by is_term_loan, amortizing, party type, customer group, territory, etc.

@bosue
Copy link
Contributor

bosue commented Sep 25, 2023

I figure that PR should now be trimmed to the minimal viable product and further functionality should be introduced in separate tickets, but at least there‘s a proof of concept now.

I‘m ready to dedicate more work to splitting it up, completing, refining and refactoring it to your wishes, if welcome and wanted at all. If left without any feedback, I will probably move my focus towards getting things done downstream though.

No doubt, you guys are very busy and normally it wouldn’t altogether be your job as maintainers. And no doubt, your platform is awesome. But community interaction is lacking, community and consensus building efforts are quite a bit lacking, subsequently decisions are made unilaterally more often than not. And that may probably be why so much work stays on the maintainers‘ shoulders and why less code is being contributed upstream than would otherwise happen. 🤷🏻‍♂️

I do intend this as criticism, but in an understanding rather than a harsh way. I'd really like to suggest that the maintainers develop less code themselves and place significantly more trust in the community, focusing on assembling the parts and creating something whole. Today, the community may be limited, particularly in new spin-offs such as lending. Tomorrow, if ppl feel invited, it may be ten times as large.

Best regards.

@anandbaburajan
Copy link
Contributor Author

@bosue thanks for the PR!

I figure that PR should now be trimmed to the minimal viable product and further functionality should be introduced in separate tickets, but at least there‘s a proof of concept now.

Yep, the overall structure looks good. As you said, can you trim both Loan Type and Loan Product to their current features?

I‘m ready to dedicate more work to splitting it up, completing, refining and refactoring it to your wishes, if welcome and wanted at all.

😄

But community interaction is lacking, community and consensus building efforts are quite a bit lacking, subsequently decisions are made unilaterally more often than not.

I agree for frappe/lending, but curious to know if you feel the same way about frappe/frappe and frappe/erpnext too.

I'd really like to suggest that the maintainers develop less code themselves and place significantly more trust in the community, focusing on assembling the parts and creating something whole.

I agree; it's just that this is kind of a new project hence we weren't expecting an enthusiastic contributor this soon.

Still need to figure out if and how documents can be filtered for containing particular MultiSelect values (i.e. for being tagged as x,y or z). Then this mechanism would allow us sufficient means for categorizing Loan Types by is_term_loan, amortizing, party type, customer group, territory, etc.

Can you open an issue at frappe/frappe?

@anandbaburajan
Copy link
Contributor Author

Closing in favour of #54.

@bosue
Copy link
Contributor

bosue commented Sep 25, 2023

@anandbaburajan:

Yep, the overall structure looks good. As you said, can you trim both Loan Type and Loan Product to their current features?

Absolutely. I would take out various amortization types and the tiered price lists which both add quite a bit too much complexity though needed in the end product. For now, we‘d keep simplifying term loans as being linearly amortized to 0.

The concepts of a base / refinancing rate with minimum/maximum margin going on top, of limited availability and of a product lifecycle I tend to consider part of the minimum viable product. Also, they‘re rather simple.

I agree for frappe/lending, but curious to know if you feel the same way about frappe/frappe and frappe/erpnext too.

It's way better there, but still I think that there‘s a lot of room for improvement. See, I super adore your company‘s super-flat hierarchy team spirit and am impressed by the results. At the same time I can imagine just because you may call such a cozy place “home“, you guys may tend to be self-sufficent making it just a bit harder for outsiders to integrate with the Frappe community.

Many years with the Drupal community made me accustomed to quite intensive community building and very careful if sometimes lengthy consensus building (or arguments weighing) efforts, resulting in a set of collectively supported major projects that in the end will move forward massively with all hands on. I can see that your more agile, more liquid, more atomized way of doing things has its upsides as well. But then you need a bit more opportunities for devs to get together, get to know each other, coordinate their efforts or at least to develop a more or less common plan of what we want to achieve at all.

How to move forward? I think having this discussion is already a step forward. Technically, allowing discuss.frappe.io to mesh a bit more with the rather barebones, code-only GitHub repos or maybe with the GitHub Projects feature would be quite some improvement. And some more focus on simplifying good quality contributions: more focus on coding standards, on Frappe’s code philosophy, also testing needs to be a no-brainer. Finally, to the very day I seriously have not yet figured out what kind of documentation links are wanted by the linter tool: what to do and when, where to put and how, where to find what others have changed/proposed with a PR etc. 🤭

Best regards!

@bosue
Copy link
Contributor

bosue commented Sep 26, 2023

@anandbaburajan
Copy link
Contributor Author

@bosue thanks for your thoughts on community building. I'll share it internally and see what we can do.

@bosue
Copy link
Contributor

bosue commented Sep 26, 2023

thanks for your thoughts on community building. I'll share it internally and see what we can do.

I appreciate it very much!

Still unsure how to best mesh discuss.frappe.io with GitHub, though I consider this the most promising approach. Most devs will almost forget about discuss.frappe.io as no real progress in terms of coding is made there. Same here, I have to admit. But then it’s lost on devs as an actual GitHub extension, which is a pity. Maybe make sure there’s a column with weekly commit logs, a release schedule and possibly a list of prioritized issues we’re specifically asking for help for etc. Maybe find a way how to prominently link and mesh a particular issue or PR with a corresponding discussion on discuss.frappe.io, so we may seamlessly continue Github discussions on discuss and come back with a result?

Just ideas… 🤗

People will certainly come up with even more helpful suggestions, of which, of course, not everything would be doable or desirable. An official, time-limited, wishlist/questionary posted on discuss.frappe.io would be awesome! The best moment will probably be v15‘s fist Release Candidate to draw in some more help for polishing a few last tidbits and fixing some remaining problems.

@anandbaburajan
Copy link
Contributor Author

@bosue about the order, can you first raise a PR to rename Loan Type to Loan Product, and once it's merged, you can raise another PR to create a new Loan Type doctype? That will help with keeping some migration headaches away. Once those are done, we can look into adding more things including #51. What do you think?

@anandbaburajan
Copy link
Contributor Author

@bosue I've shared your thoughts internally. I think you can also create a post and share on the forum. Also, you can join https://t.me/frappe_contributors if you haven't already.

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.

None yet

3 participants