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

NEW: added column default_rib in llx_user_rib #30850

Merged

Conversation

atm-thomasb
Copy link
Contributor

NEW|New column default_rib in llx_user_rib as requested in a TODO

In htdocs/compta/prelevement/class/bonprelevement.class.php, a TODO requested the following (see image):
image

In order to do that, we need to add a "default_rib" column to llx_user.

THIS COLUMN IS DEFAULTED AS 1 TO ALLOW RETROCOMPATIBILITY. The column isn't used yet so it was set to default at 1 to make sure the newly added condition doesn't mess with existing processes.

This PR is made to prepare a future PR for this feature request about RIB : #30810

@@ -87,6 +87,7 @@ ALTER TABLE llx_c_hrm_public_holiday ADD UNIQUE INDEX uk_c_hrm_public_holiday(en
ALTER TABLE llx_c_hrm_public_holiday ADD UNIQUE INDEX uk_c_hrm_public_holiday2(entity, fk_country, dayrule, day, month, year);

ALTER TABLE llx_societe_account ADD COLUMN date_last_reset_password datetime after date_previous_login;
ALTER TABLE llx_user_rib ADD COLUMN default_rib smallint NOT NULL DEFAULT 1;
Copy link
Member

Choose a reason for hiding this comment

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

If you set DEFAULT 1 in sql, it means when we will be able to insert new lines, they will have default set automatically.
May be you can set "DEFAULT NULL".
Then run UPDATE set default_rib = 1 where default_rib IS NULL
so all existing lines, at migration time, will have default_rib to 1, and then later, when we will enter a new bank, it will not be set as the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your time!

After discussing with the team and our client we realized that issue #30810 isn't about user ribs, so doing this is bonus.
With that in mind and since I'm running out of time this sprint, I won't be able to properly develop a work logic around user ribs soon. (ie: setting which ribs are to be used by default for each user, allowing to change it, etc.)

We then decided to do the following:

  • I reverted the code NOT to use "default =1" in the sql request
  • I STILL create the column and set the default to 0 (see below for explanation).
  • I updated the TODO comment to say that the column has been created if someone wants to do the work logic.
    I may end up doing it in the future, but I don't want to promise it so at least it's one step done

Why not setting default to NULL ?
--> because it's supposed to be working the same way as default_rib from societe_rib, which is NOT NULL, and by default 0

Why not using UPDATE to set 1 to all existing so we can still add the sql "AND sr.default_rib = 1"?
--> It would work, but only at first. Since we have no work logic for users yet, creating a new user would set its value to 0, while having every old user to 1, with no way to modify it. In the sql request it wouldn't be able to select the new user.

@eldy eldy added the PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do) label Sep 4, 2024
@eldy eldy merged commit 63c54a0 into Dolibarr:develop Sep 6, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants