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

⬆️ Update Grocy to v3.2.0 #284

Merged
merged 2 commits into from
Mar 22, 2022
Merged

Conversation

malcp
Copy link
Contributor

@malcp malcp commented Feb 22, 2022

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
Copy link
Contributor

oli-f commented Mar 14, 2022

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 }}';

@malcp malcp force-pushed the bump-to-3.2.0 branch 2 times, most recently from ed3ffa6 to fea1fe8 Compare March 14, 2022 15:48
@jlsjonas
Copy link

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

@frenck
Copy link
Member

frenck commented Mar 22, 2022

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

@frenck
Copy link
Member

frenck commented Mar 22, 2022

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

@jlsjonas
Copy link

jlsjonas commented Mar 22, 2022

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
Copy link
Member

frenck commented Mar 22, 2022

@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
Copy link

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
Copy link
Contributor Author

malcp commented Mar 22, 2022

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

  • The dependency bumps were required by Grocy v3.2.0, otherwise it wouldn't work.
  • Catalan language was introduced in Grocy v3.2.0, so I thought it would make sense to enable it in this PR.
  • There is no bug fix. Updating fix_braindamage.patch is needed because Grocy's codebase changed. Failing to do so would cause a regression.

@malcp
Copy link
Contributor Author

malcp commented Mar 22, 2022

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

@frenck frenck added the dependencies Upgrade or downgrade of project dependencies. label Mar 22, 2022
@frenck
Copy link
Member

frenck commented Mar 22, 2022

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 frenck changed the title Bump up Grocy version to v3.2.0 ⬆️ Update Grocy to v3.2.0 Mar 22, 2022
@frenck frenck closed this Mar 22, 2022
@frenck frenck reopened this Mar 22, 2022
@frenck
Copy link
Member

frenck commented Mar 22, 2022

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

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @malcp 👍

@malcp
Copy link
Contributor Author

malcp commented Mar 22, 2022

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.

@frenck frenck merged commit c624f64 into hassio-addons:main Mar 22, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Upgrade or downgrade of project dependencies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to set Catalan localization
4 participants