Skip to content

Conversation

@JonBendtsen
Copy link
Contributor

@JonBendtsen JonBendtsen commented Dec 7, 2025

FIX Link member to third party - no check, no feedback 26864 - BREAKING CHANGE

This PR is a partial fix of bug 26864, it only handles the API. The bug was that one could silently overwrite an existing link between memberA and a thirdpartyA by specifying a new link to thirdpartyB. This PR introduces bans direct thirdparty links when there is an existing link and instead reports back that the user has to use a 2 step process by first setting the link to 0, aka remove the link, and then introduce the new link.

Breaking Change

You can no longer just update socid using the API, you have to use a 2 step process by first setting it to 0, and then setting it to the new value

@the-dolibear-bot-for-v18 the-dolibear-bot-for-v18 bot added the Issue for v18 maintenance Team PR is in a maintenance branch with several approvers. Waiting approval of all of them. label Dec 7, 2025
@JonBendtsen
Copy link
Contributor Author

JonBendtsen commented Dec 7, 2025

I've run into some questions that needs to be determined.

First question

I'm adding a check for users access to the thirdparties included, and that is always granted. I think this is due to this check missing in v18, that I only added in v22. https://github.com/Dolibarr/dolibarr/pull/36408/files

Do we want to backport that fix to v18, do we want to ignore it? Or do we want to fix is from some other version between v18 and v22?

Second question

At first If I try to break the thirdparty link with a PUT with a socid=0, then nothing happens, most likely this right here:
https://github.com/Dolibarr/dolibarr/blob/18.0/htdocs/adherents/class/adherent.class.php#L823

Do we want to support the API breaking a link to a thirdparty? Either using the PUT or do we really want to make a special endpoint just for that? So far I "fixed" it in PUT with using $member->setThirdPartyId(0) but we could probably also remove that check in Line 823

sub decision

I decided to ban direct from thirdpartyA to thirdpartyB updates, forcing a 2 step process by first resetting and removing the link with socid=0, and then the 2. step would be to update again with the new thirdparty value.

test cases

memberA and thirdpartyA not linked both with emailA + thirdpartyB with different email

works

from any socid, update memberA with socid=0
from socid=0, update memberA with thirdpartyA or thirdpartyB works fine

fails

from socid=thirdpartyA, update memberA with thirdpartyB fails, stating use socid=0
from socid=thirdpartyB, update memberA with thirdpartyA fails, stating use socid=0

work arround

first set it to socid=0 and then to new socid

@JonBendtsen JonBendtsen marked this pull request as ready for review December 7, 2025 21:15
Copy link
Contributor

@rycks rycks left a comment

Choose a reason for hiding this comment

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

hello @JonBendtsen
i'm sorry but i'm quite sure that such PR could not be integrated into 18.0 because as you says it introduce a break ... i share your point of view "that's a bug" but as a merger
we can't apply it i think ... but maybe @eldy will have an other point of view ?

https://wiki.dolibarr.org/images/d/d9/Chart_of_decision.png

@JonBendtsen
Copy link
Contributor Author

hello @JonBendtsen i'm sorry but i'm quite sure that such PR could not be integrated into 18.0 because as you says it introduce a break ... i share your point of view "that's a bug" but as a merger we can't apply it i think ... but maybe @eldy will have an other point of view ?

https://wiki.dolibarr.org/images/d/d9/Chart_of_decision.png

Hey @rycks. I appreciate that you are concerned about breaking changes, I am too, which is why I mentioned that this was a breaking change. I'll wait for @eldy's input before making any changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue for v18 maintenance Team PR is in a maintenance branch with several approvers. Waiting approval of all of them.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants