hassio-addons / addon-grocy

Grocy - Home Assistant Community Add-ons
https://addons.community
MIT License
319 stars 60 forks source link

⬆️ Update Grocy to v3.2.0 #284

Closed malcp closed 2 years ago

malcp commented 2 years ago

Proposed Changes

Bumped Grocy version to v3.2.0. The addon was tested following the "Local Add-on Testing" in https://developers.home-assistant.io/docs/add-ons/testing/

Related Issues

oli-f commented 2 years ago

I just noticed that the fix_braindamage.patch patch fails due to this commit.

I have adapted and tested the patch:

diff --git a/views/layout/default.blade.php b/views/layout/default.blade.php
index 4eb3fb8d..d4e5e007 100644
--- a/views/layout/default.blade.php
+++ b/views/layout/default.blade.php
@@ -88,6 +88,10 @@
        Grocy.Mode = '{{ GROCY_MODE }}';
        Grocy.BaseUrl = '{{ $U('/') }}';
        Grocy.CurrentUrlRelative = "/" + window.location.href.split('?')[0].replace(Grocy.BaseUrl, "");
+       if(Grocy.BaseUrl.indexOf('http') != 0 || Grocy.BaseUrl.indexOf('hassio_ingress') >= 0) {
+           Grocy.BaseUrl = window.location.origin + '{{ $U('/') }}';
+           Grocy.CurrentUrlRelative = "/" + window.location.pathname.replace(Grocy.BaseUrl, "");
+       };
        Grocy.ActiveNav = '@yield('activeNav', '')';
        Grocy.Currency = '{{ GROCY_CURRENCY }}';
        Grocy.CalendarFirstDayOfWeek = '{{ GROCY_CALENDAR_FIRST_DAY_OF_WEEK }}';
jlsjonas commented 2 years ago

Is anything blocking this (besides @frenck's time)?

frenck commented 2 years ago

@jlsjonas working on the add-on as we speak...

frenck commented 2 years ago

This PR does contain multiple things that don't belong in a single PR. So it won't be merged though.

jlsjonas commented 2 years ago

Apart from the unfortunate commit message naming the diff feels quite in scope? (bumping grocy & fixing related items + patch-bumping used php8 modules as part of a general version bump feel quite related)

But your call ;) not my PR :)

frenck commented 2 years ago

@jlsjonas it isn't. It contains a bug fix, an Grocy upgrade, a dependency bump and support for a new feature.

I appreciate one tries to help, but PR should never have multiple concerns squashed together.

jlsjonas commented 2 years ago

Agreed and misconception on my end, I thought the patch fix was due to the version bump. I do fail to see the new feature though (ca language? Didn't that come from the update?)

malcp commented 2 years ago

Sorry if I mixed things up. These were my reasons:

malcp commented 2 years ago

I'm happy to modify/split this PR if that would unblock it from getting merged.

frenck commented 2 years ago

I've rebased the PR and pulled out the dependency upgrades that way.

Let's go ahead and merge the rest in one. In general, it's nice for someone to read "Catalan" was added as a language (or other changes that are made). Those will be hidden in a single upgrade message (which is not transparent).

frenck commented 2 years ago

(close/re-open was to re-trigger CI, GitHub was having some hickups earlier today)

malcp commented 2 years ago

I've rebased the PR and pulled out the dependency upgrades that way.

Let's go ahead and merge the rest in one. In general, it's nice for someone to read "Catalan" was added as a language (or other changes that are made). Those will be hidden in a single upgrade message (which is not transparent).

Great! And fair point, I'll keep this in mind for next time.