timschofield / webERP

webERP Accounting and Business Administration ERP system
https://www.weberp.org
GNU General Public License v2.0
29 stars 113 forks source link

GetConfig.php lines 43 to 50 #248

Closed pakricard closed 6 days ago

pakricard commented 1 week ago

Do we really need to keep code lines like these lines 43 to 50?

    /*
     check the decimalplaces field exists in currencies - this was added in 4.0 but is required in 4.04 as it is used everywhere as the default decimal places to show on all home currency amounts
    */
    $Result = DB_query("SELECT decimalplaces FROM currencies",'','',false,false);
    if (DB_error_no()!=0) { //then decimalplaces not already a field in currencies
        $Result = DB_query("ALTER TABLE `currencies`
                            ADD COLUMN `decimalplaces` tinyint(3) NOT NULL DEFAULT 2 AFTER `hundredsname`");
    }

decimalplaces was added ages ago, and we have this random check here, but does it make sense to keep it (or similar ones)?

My vote is to delete them.

dalers commented 1 week ago

I don’t claim to understand this, but it seems the code is to allow for an old db schema. Do I understand correctly? There seems to be an unwritten rule that databases MUST be updated to a newer schema (i.e, old schemas are not supported), and if so, then it would seem code would never have to accommodate an old schema. I.e. this code would be redundant. Am I missing something?

pakricard commented 1 week ago

@dalers : Yes, this code fixes the DB in case currencies table doesn't have the decimalplaces field. If we do it here, why not everywhere? It seems illogical to me. And all scripts should assume that the DB version is the proper one for the code, except the "Update Database", of course.

dalers commented 1 week ago

Then my vote is also to remove code like this.

What should be the process for making such a change? I think it would be best to complete removing all "old" decimal places code in a development branch for clarity and ease of testing, instead of multiple incremental commits to the master branch. However, I'm not sure if the development branch should be done in a personal fork or in the master repo (i.e. Tim's repo). Maybe the solution depends on how significant the work is and whether there will be multiple developers working simultaneously (in which case a dev branch in the master repo would be the most convenient).

@timschofield how do you envision the project management process in a situation such as this?

pakricard commented 1 week ago

Hi @dalers , I think just these lines on the initial post need to be removed

dalers commented 6 days ago

Hi @pakricard I guess it's a simple commit then. Can you submit a pull request to https://github.com/timschofield/weberp from your fork?

Do our new permissions from @timschofield give us the ability to accept pull requests? If so, I could accept the pull request but for now and for me personally, I'd prefer Tim continues as the only person who merges into the master branch (i.e. Tim is to webERP as Linus Torvalds is to the Linux kernel)..

like these lines 43 to 50?

Is it worth searching the project for similar code ("like these lines")? I expect however you don't mean there are many similar examples in the project, but rather there are many chunks of code that should be deleted, and this is only one example of many and they are all different.

Btw, are you using Filezilla in real time at a directory level while you are editing using Notepad++? I.e. does Filezilla automatically update files on your server as you edit and save them locally? If the only "project" tool you really need is project-level search and replace, then imho Filezilla+Notepad++ seems a pretty clean solution without the overhead of NetBeans (or PHPStorm). Do you want more? What? E.g. do you want to single-step code to follow an SQL query being constructed?

dalers commented 6 days ago

P.S. I read a recommendation for "sshfs-win" (https://github.com/winfsp/sshfs-win). Iiuc, you could use sshfs-win to create a local drive letter on your Windows system that is actually a directory on your server, and you would be able to edit the files using Notepad++. It would omit the need for filezilla but I haven't tried it personally though....

pakricard commented 6 days ago

This is great! I managed to commit directly into upstream (a.k.a. Tim's master repository) from my SourceTree. Moreover, adding the text “Fixes #xxx” into the commit text closed the issue automatically in GitHub.

So I think Tim's restored my ancient rights into the repository (or were never deleted, and I managed to make them work again). Thanks!

dalers commented 6 days ago

That's great @pakricard ! :-)