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

[mainui] Fix addons details serviceId #1202

Merged
merged 4 commits into from
Nov 14, 2021
Merged

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Nov 8, 2021

The serviceId for details page in the addon store was not properly determined if the addonId did not start with marketplace:.

@J-N-K J-N-K requested a review from a team as a code owner November 8, 2021 17:17
The serviceId for  details page in the addon store was not properly determined if the addonId did not start with marketplace:.

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
@relativeci
Copy link

relativeci bot commented Nov 8, 2021

Job #253: Bundle Size — 10.75MB (~+0.01%).

56e0f9d vs 0a5c9e1

Changed metrics (0/8)

No changes

Changed assets by type (1/7)
            Current     Baseline
JS 8.47MB (~+0.01%) 8.47MB

No changes


View Job #253 report on app.relative-ci.com

@ghys
Copy link
Member

ghys commented Nov 9, 2021

You might want to address this problem too eventually:

value: (marketplace) ? 'Community Marketplace' : 'openHAB Distribution'

i.e. every add-on not coming from the marketplace will be advertised as coming from the openHAB distribution.
This was to avoid a call to /rest/addons/services to get the label. Though it might be sensible to do that:

  • serviceId=marketplace => Source: Community Marketplace
  • serviceId=karaf => Source: openHAB Distribution
  • otherwise: use the Id as the source, or call /rest/addons/services and find out the label asynchronously.

If you want to postpone that to another PR I can accept this one in the meantime.

@J-N-K
Copy link
Member Author

J-N-K commented Nov 9, 2021

The question is how to fix that. I must admit that I have no idea how to do requests.

Of course it is easy to change it to indexOf(':') > 0 but that is not a proper fix either, because then other addon providers would identify as "Community Marketplace". If you think that is better, I'll happily update the PR. Another solution would be to distinguish between "community mp", "distribution", "other". This is possible without querying.

I did a quick check that the marketplace is used in three lines:

l. 50: to determine the "source"
l. 81: ?
l. 103: to add details like documentation.

l.50 could be changed to "Community Marketplace" (if starts with marketplace:), "openHAB Distribution" (if no prefix) and "Other" (if other prefix).
l. 103: a third case could be added, "documentation link" -> "addon.link".

Unfortunately I have no idea what l. 81 does. If you give me a hint, I can make the adjustments for this.

Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented Nov 13, 2021

@ghys I tried to find a solution. WDYT?

Signed-off-by: Jan N. Klug <[email protected]>
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

LGTM - both the solution and the code.
Eventually the "3rd Party (serviceId)" could be improved with a request to get the real label but it's not that important right now.

@ghys ghys merged commit 68d3ef4 into openhab:main Nov 14, 2021
@ghys ghys added bug Something isn't working main ui Main UI labels Nov 14, 2021
@ghys ghys added this to the 3.2 milestone Nov 14, 2021
@J-N-K J-N-K deleted the fix-marketplace branch November 14, 2021 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants