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

[transform.vat] Initial contribution #14529

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Mar 3, 2023

This simple transformation adds VAT (Value-Added Tax) to a given input amount.

Its main purpose is to conveniently let the provided profile add VAT when linking an item to a channel without requiring any configuration. However, it can also be used for display purposes only, although in this case the rate must be configured.

One use-case is to eliminate the need for any built-in VAT logic in bindings providing electricity prices, thereby reducing redundancy.

Advantages:

  • Automatic rate according to country in openHAB regional settings (no configuration needed).
  • Consolidates VAT calculation in one centralized place.
  • Potentially eliminates custom VAT calculations in bindings.
  • Not dependent on any online service.

Limitations:

  • Only standard rate supported, no specialized types of VAT.
  • Only one rate per country supported, no deviations for specific territories .
  • Only current rate supported, no validity periods implemented yet.
  • Rates are currently built in, thus add-on needs to be distributed when rates are changing.

Alternatives:

  • Redundant VAT implementations in bindings.
  • Manually applying VAT through rules.
  • Using the MULTIPLY math transformation in SmartHome/J having the rate configured multiple times.

See also #14222 (comment)

Resolves #14524

Screenshots

image

image

@jlaur jlaur added enhancement An enhancement or new feature for an existing add-on new transform labels Mar 3, 2023
@jlaur jlaur requested a review from a team as a code owner March 3, 2023 22:11
@jlaur jlaur removed the enhancement An enhancement or new feature for an existing add-on label Mar 3, 2023
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/vat-transformation-4-0-0-0-4-1-0-0/144942/1

@J-N-K
Copy link
Member

J-N-K commented Mar 4, 2023

I can't find the discussion at the moment, but I remember that a "MULTIPLY" or "DIVIDE" profile was rejected for being to specific and confusing (because of too many transforms/profiles if similar ones would be introduced) and those problems should be solved in rules. This seems to be even more specific.

@jlaur
Copy link
Contributor Author

jlaur commented Mar 4, 2023

I can't find the discussion at the moment, but I remember that a "MULTIPLY" or "DIVIDE" profile was rejected for being to specific and confusing (because of too many transforms/profiles if similar ones would be introduced) and those problems should be solved in rules. This seems to be even more specific.

I was initially looking at the SmartHome/J math transformation, because the MULTIPLY profile would have been a quick solution for me to this problem.

I agree that this profile is very specialized, but personally I think both the math transformation and this one provides value. The math transformation fits right in, where this one is more in a gray area because in contains logic which might be too complex for the intention of transformation/profiles - not sure. I can't find any other types of components that would fill in the gap I'm trying to fill here, though.

I recently merged #13259 which is also very specialized - I was (and am) not aware of any guidelines for the scope of profiles and transformations. Also, no one had commented with any arguments against the PR since August 2022.

@openhab/add-ons-maintainers - any opinions/input? Also @J-N-K - as I read your comment, you are referring to a rejection reason provided by someone other than yourself. What is your own opinion here?

@jlaur
Copy link
Contributor Author

jlaur commented Mar 12, 2023

@openhab/add-ons-maintainers - gentle ping. Which level of specialization for profiles/transformations in the addons repository is acceptable? Examples:

IMHO, since this is selection of dedicated tools that users can choose to install (or not), we should try to be as open as possible. I would personally have used the Math transformation before adding VAT calculations into the Energi Data Service binding, but would also prefer this even more specialized version rather than building monolith bindings with duplicated functionality or leaving it entirely to the users to add such functionality in rules, creating even more duplication.

@lolodomo
Copy link
Contributor

IMHO, since this is selection of dedicated tools that users can choose to install (or not), we should try to be as open as possible.

I agree.

@J-N-K
Copy link
Member

J-N-K commented Mar 13, 2023

Since the add-ons list is not searchable (at least for me), you should also think about grouping similar functionality in one bundle. With increasing number of profiles the UI should be changed to a dropdown instead of the selector we have now.

Otherwise I agree that it should be as open as possible, but keeping in mind that "we don't want it to do everything for everybody".

@kaikreuzer
Copy link
Member

I agree that we should be open as there can be many helpful specific transformations for different situations.
My only concern is indeed the potential abundance of such transformations/profiles.
While bindings are a fairly complex construct that make sense to have in dedicated bundles, I feel that for a fairly simple feature like this VAT transformation, 660 locs are a lot - making it on the one hand a lot of effort to implement such an add-on and making it on the other hand a potentially large set of additional add-ons a user might want to add.

So I like @J-N-K's proposal of grouping similar things into a single transformation add-on. Like car companies went away from putting every single feature on their pricelists to offering feature bundles, we could also make the choice easier for users here. Especially, if a feature does not require any bigger external libraries, but is rather a piece of simple Java logic, we could ship many such transformations with others "for free" even, if the user might not use them (yet).

The resulting question would now only be: What are reasonable groups?

@jlaur
Copy link
Contributor Author

jlaur commented Mar 13, 2023

So I like @J-N-K's proposal of grouping similar things into a single transformation add-on. Like car companies went away from putting every single feature on their pricelists to offering feature bundles, we could also make the choice easier for users here. Especially, if a feature does not require any bigger external libraries, but is rather a piece of simple Java logic, we could ship many such transformations with others "for free" even, if the user might not use them (yet).

Makes sense. Only thing to consider here - as also pointed out by @J-N-K:

With increasing number of profiles the UI should be changed to a dropdown instead of the selector we have now.

Currently profiles which are not installed will not "pollute" the UI. If we start bundling them, more entries would also show up in the UI, even some specific ones the user may not be interested in (yet). So probably the UI should be improved before bundling too many profiles.

The resulting question would now only be: What are reasonable groups?

In this repository, this is all we currently have:

  • org.openhab.transform.bin2json
  • org.openhab.transform.exec
  • org.openhab.transform.javascript (doesn't exist anymore)
  • org.openhab.transform.jinja
  • org.openhab.transform.jsonpath
  • org.openhab.transform.map
  • org.openhab.transform.regex
  • org.openhab.transform.rollershutterposition
  • org.openhab.transform.scale
  • org.openhab.transform.vat (doesn't exist yet)
  • org.openhab.transform.xpath
  • org.openhab.transform.xslt

Possible candidates:

  • JSON: bin2json + jsonpath
  • XML: xpath + xslt

Of course, if we welcome new profiles, there will also be more to group. 🙂

Maybe you were already thinking about more abstract groups, i.e. some kind of "themes"?

@J-N-K
Copy link
Member

J-N-K commented Mar 13, 2023

There are also already the system profiles. That aside: profiles are not necessarily coupled to transformations.

Copy link
Contributor

@clinique clinique left a comment

Choose a reason for hiding this comment

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

LGTM

@kaikreuzer
Copy link
Member

That aside: profiles are not necessarily coupled to transformations.

That's right - we even do not have any "profile add-ons", but only "transformation add-ons". But every transformation add-on comes with its own profile. So bundling together transformations will automatically result in a longer list of profiles (which the user might not want).

Maybe it would already be enough to split the profiles into 3 groups in the UI: System profiles, transformation profiles and profiles that come with other add-ons (e.g. the enocean binding). This should already give some better overview.

Still, this does not address the potential proliferation of transformation add-ons itself. I don't really have any good idea how to deal with this yet, since packaging things together is imho not well supported by our architecture (wrt installation, documentation, etc.). So maybe we should take that discussion out of scope of this PR and just consider "vat" to be another fairly specific transformation next to rollershutterposition and we will deal with the matter as soon as we really feel that there's too much uncontrolled growth...

@J-N-K
Copy link
Member

J-N-K commented Mar 16, 2023

My comment was not intended as a request to delay merging, just an information for those not so familiar with profiles 😀

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@kaikreuzer kaikreuzer merged commit 4d47f33 into openhab:main Mar 17, 2023
@kaikreuzer kaikreuzer added this to the 4.0 milestone Mar 17, 2023
@jlaur jlaur deleted the 14524-vat branch March 17, 2023 22:33
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/dishwasher-price-calculation-automation/139207/5

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/vat-transformation-3-4-0-0-4-0-0-0/145451/1

@jlaur
Copy link
Contributor Author

jlaur commented Mar 22, 2023

@Confectrician - should new transformations appear here automatically, or is some manual action needed:
https://next.openhab.org/addons/

? I'm wondering if I got some kind of metadata wrong.

renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[transform.vat] Simple transformation service for adding VAT to amount
6 participants