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

Failed to add conversion which exists already in other product #707

Closed
VNRARA opened this issue May 23, 2023 · 12 comments
Closed

Failed to add conversion which exists already in other product #707

VNRARA opened this issue May 23, 2023 · 12 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@VNRARA
Copy link

VNRARA commented May 23, 2023

Removing a used unit allowed a broken unit to break my entire database and a specific unit (renamed to stuk) in my database keeps giving errors.

Screenshot_20230523_221822_Grocy

@VNRARA
Copy link
Author

VNRARA commented May 23, 2023

Longer explaination:

I added a product under parent product cheese (kaas) and it wouldn't add a convertion qu for [pak] to [stuk] (showing red)
Then I deleted the qu [stuk] which broke the database and had to re-add and fix the reference in sqlite.
Adding a conversion for these two qu (any qu with [stuk] used to work just fine until the last 1or 2 updates. )

@SapuSeven
Copy link

SapuSeven commented May 23, 2023

Similar problem here.
Im not sure what happened before, but I captured a screen recording:

screen-20230524-015219.2.mp4

The item has a QU of "Bottle".
When trying to add a QU conversion from "Bottle" to something else, every other unit works fine except "Liter".
When choosing a different from-unit, "to Liter" works as expected.
EDIT: Also it's worth mentioning that I was able to add the conversion from Bottle to Liter in the Grocy Web UI without any problems.

@VNRARA
Copy link
Author

VNRARA commented May 24, 2023

It's absolutely terrible. At one moment I CAN select a specific unit and not even a closing the app or screen for qu and it stopped accepting that same from/to qu conversion.

Screen_Recording_20230524_174005_Grocy_1.mp4

@VNRARA
Copy link
Author

VNRARA commented May 27, 2023

They changed code that made the unit conversion check go globally instead of per product:

Test product one convert zak - > bak
Test product two convert zak -> bak2 (error)

Which isn't how grocy should work at all. Four days in and no response.

@patzly
Copy link
Owner

patzly commented May 27, 2023

@VNRARA We are both studying and working and this is a free-time-only project. All work we do here is fully based on our free time and motivation, and if people point on us saying that we should answer more quickly and that our work is shitty, their issues will very likely be downgraded in our priority queue as our motivation is then simply missing. I hope you understand that so you can take the pressure out of your future messages and be more kindly, or skip our project entirely if you don't appreciate our time and work.

@VNRARA
Copy link
Author

VNRARA commented May 28, 2023

I try but some changes and your (non-)reactions make me really mad. We just want acknowledgement when we report things even if you can't fix it right away. I really want to help and clearly my passion for this got mistaken for ungratefulness @ #703 . I've contemplated reworking the code of the app and presenting PR's (after learning how). Looking at adding JUnit to prevent regression bugs, but I feel that discussion or other ideas are not wanted here.

@SapuSeven
Copy link

I think you need to keep in mind that even though this project is open source, it still belongs to someone. If you feel that their decisions are wrong, it's important that you don't demand anything but rather suggest changes and be open to discuss it.
After all, you get all this work for free, so if there is any bugs like this, there's no obligation to the developers to respond instantly. It will be addressed when there's time.

@VNRARA
Copy link
Author

VNRARA commented May 28, 2023

Okay, I code myself and see how much PRs these guys will accept. 👍🏽But most likely it will be responded with "Creating such a PR is ridiculous"

@dominiczedler
Copy link
Collaborator

But most likely it will be responded with "Creating such a PR is ridiculous"

We will see. ;)

Feature contributions or bug fixes are highly welcome. I know that the code of this app is not the best in the world, so I hope it is possible to understand for other people.

I will investigate the bug in the next days, all I can say for now is that there was no intention in breaking things. It could be a mistake in one commit, which was meant as improvement for the "error issue" where always a network error was displayed if the conversion exists (catch wrong form content in the UI before the server answers with error).

@VNRARA
Copy link
Author

VNRARA commented May 29, 2023

Thanks man :) Also for putting up with my sh*t.

@dominiczedler dominiczedler self-assigned this Jun 4, 2023
@dominiczedler dominiczedler added the bug Something isn't working label Jun 4, 2023
@dominiczedler dominiczedler changed the title Removing a unit in use allowed a broken unit to break my entire database Failed to add conversion with exists already in other product Jun 4, 2023
@dominiczedler
Copy link
Collaborator

dominiczedler commented Jun 4, 2023

I've hopefully fixed the bug now.

It was in a method (used in form validation) that should find conversions for a given From-QU and To-QU together with a productId. As there are also global QU conversions it should return a global one (if it exists) if the product conversion doesn't exists. The mistake was that it didn't filter for global conversions (which don't have a productId in database) but instead looked for any other conversion with these From-QU and To-QU. In your cases you likely have the conversion which failed already in your database in another product. This should work again in the next release or the upcoming nightly build (which you could test to give feedback).

@VNRARA The bug has nothing to do with your original title of the issue. I don't understand what you have made exactly but I can say that our app is not the source of the broken database. It is the API of grocy server which should prevent that and give a HTTP error response then. If it was not the case in your setup and actions, then maybe the API needs some (more) checks for a specific endpoint. If you want to report this in grocy server repository, you should make the actions more reproducible (with english demo server).

@dominiczedler dominiczedler added this to the v3.3.0 milestone Jun 4, 2023
@VNRARA
Copy link
Author

VNRARA commented Jun 6, 2023

_

@VNRARA VNRARA changed the title Failed to add conversion with exists already in other product Failed to add conversion which exists already in other product Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants