Fix item number lookup in sales/receivings (#4212)#4250
Conversation
jekkos
force-pushed
the
fix-item-number-scan
branch
from
b737148 to
8938e81
Compare
|
That's correct , the code is a bit messy. It uses item_id to store both item number and id. So it can be string or int. I can rename the variable to make that clear |
|
OK that makes sense. another way, in addition to renaming is to make the data type of the parameter a union type. IMO this makes it clearer that both types are accepted. e.g., |
|
Also I think we said that as we modify the code we are going to refactor function and variable names from snake_case to camel case for PSR-12 compliance. Are you up for that refactor in these few sections of code? |
|
I just checked PSR12 and it recommends to have function names in camelCase, but there is no recommendation for variables. |
|
PSR12 does not explicitly require camelCase variable names, and when referencing database fields, it's not even possible, but I argue that where possible, we should agree to variable names using camelCase for consistency with the naming convention for methods. A second argument is that this matches CI4 coding conventions so all the files we inherit from CodeIgniter will be camelCase variables. A third argument is a guess that I haven't verified, but I won't be surprised if IDE linters complain about variable names not being camelCase. I know this isn't the same as a specific declaration, but are there any variable names in the PSR12 OR PSR1 example code that show camelCase or snake_case? |
|
@objecttothis fine by me to change it, but I would try to use an AI to do this conversion. currently I am a bit time constrained and wanted to get this issue fixed. Let's log a ticket for the conversion so it can be tackled after. |
|
I would like to release a 3.4.1 soon as many fixes made it to master already. would be nice to have the mysql one as well. Then we can do more cleanup after |
|
That is fine to punt it to another PR. |
Without the migrations fixes that I have a PR out for, anyone running MySQL will still not be able to upgrade. I'm almost done with my PR. I'll try to get it to a stopping point tomorrow but I could really use someone testing the changes with MariaDB. The only two changes I have left are to add the migration file to kick off one of the migration SQL's and then test my code changes to make sure we can still delete a dropdown. I'm traveling June 9 - August 20, so I too want to get this into a good place to release 3.4.1, but I don't think we should release anything that MySQL users can't install. |
|
Agree let's try to get this sorted. If you want just push the changes to a branch here in the repo and it will auto deploy. Then we can call for some testing. We use Mariadb for the dev server so it will be clear if it starts correctly from scratch or not. It runs all the migrations after every deploy so that is a good starting point. Still need to give you acces to it at some point. Sounds like you will have some nice time oiff then June onwards, where are you heading? |
My family and I are headed to the US to see family and friends. I'll have a bunch of other work to do mixed with some fun things like a backpacking trip, but I won't have time to write code. I'll try to answer questions where I can though. #4230 is ready for review and if all is good, we can get both these merged. In the time I have left before travel, I'll probably start work on the framework for CodeIgniter Events in our code which will pave the way for 3rd party integrations. |
I changed #4230 from a draft to a ready pull request and pushed changes, but it says it hasn't deployed. I wonder if because you had requested changes a few weeks back it doesn't deploy. |
|
Ah no the github status is not linked to the dev server deployment, I never looked into that in fact. The deploy is done when travis pushes the final docker image for a commit. It uses a webhook in docker hub if I'm not mistaken. |
|
cae921 Currently this commit is deployed, but it's on MariaDB. The migrations seem to have ran |
jekkos
force-pushed
the
fix-item-number-scan
branch
from
f285901 to
051a7aa
Compare
|
I have pushed the commit for this branch again and reworked a bit by looking at how this work for sales. It's similar now so more consistent. I tried to add both an id and item_number and both worked fine. I'll merge this and then we can release, unless if someone finds another issue. |
|
@objecttothis so it's ready for review, if you want. |
I'll take a look |
objecttothis
left a comment
objecttothis
left a comment
There was a problem hiding this comment.
Just the one comment about changing the return from bool to string. Unless there is a good reason, I would change it back. I think we also talked about refactoring the naming of item_id in functions where it could be an item_id or an item_number to a variable name that makes it more clear that it could be either. Maybe $itemReference or something similar?
|
It's quite cool, I asked copilot to convert the method to camelCase, gave me the converted version. Also found that VSCode might be able to go one step further and there you can integrate an AI agent that can then even do the update itself. The plugin is called cline. Copilot can be used as a linter also in this case, I asked it to check the whole file and it gave me quite some suggestions |
jekkos
force-pushed
the
fix-item-number-scan
branch
from
051a7aa to
19e5164
Compare
Does it only refactor the local variables or does it also refactor function names? I've been nervous to ask AI to do the refactoring out of fear that it won't refactor the function definition or all the usages. That and PHPStorm seems to do a pretty good job at the refactoring... Just not automatic. |
In PR #4250 (commit 29c3c55), orWhere was added to match items by either item_id or item_number, but the OR condition was not wrapped in groupStart()/groupEnd(). This causes: 1. Wrong SQL semantics: generates WHERE item_id = ? OR item_number = ? AND deleted = 0 instead of WHERE (item_id = ? OR item_number = ?) AND deleted = 0 Due to AND binding tighter than OR, the deleted filter only applies to the item_number branch, allowing deleted items to match via item_id. 2. Performance: the unscoped OR causes MySQL to bypass the item_id primary key index and fall back to full table scans when item_number is a string column compared against a numeric parameter. Both exists() and get_item_id() are fixed by wrapping the OR conditions in groupStart()/groupEnd() for proper parenthesization.
…4520) In PR #4250 (commit 29c3c55), orWhere was added to match items by either item_id or item_number, but the OR condition was not wrapped in groupStart()/groupEnd(). This causes: 1. Wrong SQL semantics: generates WHERE item_id = ? OR item_number = ? AND deleted = 0 instead of WHERE (item_id = ? OR item_number = ?) AND deleted = 0 Due to AND binding tighter than OR, the deleted filter only applies to the item_number branch, allowing deleted items to match via item_id. 2. Performance: the unscoped OR causes MySQL to bypass the item_id primary key index and fall back to full table scans when item_number is a string column compared against a numeric parameter. Both exists() and get_item_id() are fixed by wrapping the OR conditions in groupStart()/groupEnd() for proper parenthesization. Co-authored-by: Ollama <ollama@steganos.dev>
4 participants
No description provided.