FIX #32791 When updating task details, keep original fields when they are not shown in the task table

[mdeweerd]

FIX #32791 When updating task details, keep original fields when they are not shown in the task table

This fixes lost user and THM when updating task time, and other lost fields.

The task time was assigned the user '0' when updating, which resulted in an error where a NULL object was used.
This resulted in a partially replaced record with no user and empty THM.

The correction adds the use of a DB transaction to avoid inconsistency in case of an error.
The correction uses the fields from the original record if no new fields are provided in the POST arguments

[hregis]

@mdeweerd GETPOSTINT return 0 if the value is null, you can use GETPOST(xxxx, 'int') instead

[mdeweerd]

@mdeweerd GETPOSTINT return 0 if the value is null, you can use GETPOST(xxxx, 'int') instead

The code first validates that the value is provided and not '' before using the int value.
I normally would use intermediate variables to get the POST value and then reuse that, but that's not the method used in dolibarr. In that case, $val = GETPOST(xxx, 'int'); if($val!=='') {$otherval=(int)$val;}

Anyway, this code works for me: I can update a line. I also tested on a line that I fixed in the DB without updating the cost value (which was 0) and the cost was updated to the correct value when I saved the line (without any modifications made).

[mdeweerd]

Draft again, some cases are not ok.

[vsatmydynipnet]

I fixed the one affecting me. Commented the patch, but I am sure there is a better solution :-)

--- project.class.php.ORIG      2025-02-12 12:33:32.468000000 +0100
+++ project.class.php.01.VS    2025-02-12 13:44:31.652000000 +0100
@@ -1620,9 +1620,22 @@
                $sql .= " WHERE p.entity IN (".getEntity('project').")";
                // Internal users must see project he is contact to even if project linked to a third party he can't see.
                //if ($socid || ! $user->rights->societe->client->voir) $sql.= " AND (p.fk_soc IS NULL OR p.fk_soc = 0 OR p.fk_soc = ".((int) $socid).")";
-               if ($socid > 0) {
+                // -----------------------------------------------------------------------------------
+                // VS There must be a bug:
+                // socid is the socid of the contact, probably only if the dolibarr user is linked to a business partner contact?
+                // then the query would want to select p_fk_soc based on the fk_soc of the partner linked to the user
+                // and it fails with 0 returns. For whatever reason this does not happen in 19.0.2, but started with 20.0.3
+                // our user, linked to a business partner, is named as external contact, maybe because of the link. So he has permissions,
+                // but there are no records selected because p-fk_soc is set based on the customer the project is opened and not
+                // on the fk_soc of the user (and his linked to partner)
+                // NOTE: socid is set also if user is linked to a business partner and therefor he is external. then socid is set with the one of the user
+               // Buggy if ($socid > 0) {
+                // I am sure there is a better solution....
+               if ( ($socid > 0) AND ($user->socid != $socid) ) {
                        $sql .= " AND (p.fk_soc IS NULL OR p.fk_soc = 0 OR p.fk_soc = ".((int) $socid).")";
                }
+                // end of fix
+                // -----------------------------------------------------------------------------------

                // Get id of types of contacts for projects (This list never contains a lot of elements)
                $listofprojectcontacttype = array();
@@ -1665,7 +1678,7 @@
                        $sql .= $filter;
                }

-               //print $sql;
+               //print "FUNCTION SQL:" . $sql."<br>";

                $resql = $this->db->query($sql);
                if ($resql) {

For me this fix works for my problem.

[mdeweerd]

Did you check if the task in the table still has the user after the change?

The patch changes the documented functionnality of the method - it doe nos longer filter on a specific third party if the third party that is examined is the same as the one of the user.
This indicates that getProjectsAuthorizedForUser influences your issue, so one needs to check how.

[vsatmydynipnet]

As I wrote I commented the patch and I am sure there are better solutions.

My problem was:

• As user which is linked to some business partner and shown as external, has rights to the project/task
• The project is linked to another business partner
• Based on the above the user was able to enter time into a project, but the line was not shown anymore after they added the line. So my users made massive troubles because they entered time in a "blind way" unable to correct typos

Currently some users added times and it looks good. The function by itself, as I understood, only return projects the user has rights too as string or array?

[mdeweerd]

In the latest code this was added versus 19.0.2, so maybe that is your reason or your fix:

htdocs/projet/tasks/time.php

        if (!$user->hasRight('projet', 'all', 'lire')) {
            // Get list of project id allowed to user (in a string list separated by comma)
            // TODO This may generate performance trouble when list of project is very large. Solution can be 
o complete $filterproj with filters on project.
            $filterproj = '';
            $projectsListId = $projectstatic->getProjectsAuthorizedForUser($user, 0, 1, $user->socid > 0 ? $user->socid : , $filterproj);
            $sql .= " AND p.rowid IN (".$db->sanitize($projectsListId).")"; // public and assigned to, or restricted to company for external users
        }

[vsatmydynipnet]

That code was my start investigating.

$projectsListId = $projectstatic->getProjectsAuthorizedForUser($user, 0, 1, $user->socid > 0 ? $user->socid : , $filterproj);

returns an empty value in my case, because sql2 query from the getProjectsAuthorizedForUser in my case is an empty result.

at least since 20.0.3 socid in the function is the socid of the business partner socid of the business partner the user is linked too and not the socid of the project/task itself.

[vsatmydynipnet]

I assume the solution would be to address the "socid named here:

•           if ( ($socid > 0) AND ($user->socid != $socid) ) {
                    $sql .= " AND (p.fk_soc IS NULL OR p.fk_soc = 0 OR p.fk_soc = ".((int) $socid).")";
            }
  

as PROJECT->socid here. I assume $socid is filled wrong in 20.x.x.

[mdeweerd]

My interpretation of the code in time.php is that this restriction is added only if the user does not have the right to see all projets and so the projects are restricted to his socid.
The modification of getProjectsAuthorizedForUser undoes this restriction.

So in this case the question is what rule allows the user access to the project + what the reason is to show the project on this page.
Mode 0 restricts access to public and assigned projects.
So the $socid argument should be use to restrict to the company viewed on the page, or the $filter argument should be used for that.

Apparently this restristion was introduced in #30362 .

[vsatmydynipnet]

I think this code does make the entry/ies available to internal users if the project is linked to a 3rd party business partner:

// Internal users must see project he is contact to even if project linked to a third party he can't see.

But I think since 20.x somewhere else $socid if filled with the id of the businesspartner the logged in user is linked too and not with the socid where the project is linked too. But I have not found where $socid without any $something->socid is set.

[hregis]

@vsatmydynipnet You should never say that your solution is the best solution or the only solution because you don't have a global vision of all the interactions! You see what you see because that's what you want to see...

[vsatmydynipnet]

@hregis where I said my code is the best solution?

I wrote:
"As I wrote I commented the patch =>and I am sure there are better solutions<=."

I was happy to fix my problem, but I am sure, somebody with deeper knowledge shoulld look into this one and fix the $socid change finally.

[hregis]

@vsatmydynipnet ah ? sorry i don't understand your words !

"I fixed the one affecting me. Commented the patch, but I am sure there is a better solution :-)"

[mdeweerd]

Putting into draft again, there still is a case where there is an issue.

[mdeweerd]

The other issue occured when I tried to update the start time. I think I had already seen this before.

The fix is to update both the timespent_date and timespent_datehour fields. It's unclear why we need both AND the timespent_withhour.

I also added the taskid (fk_element) for editline to allow verifying it was update. As it was not provided it was always deleting the entry first, so now it updates the entry. Assigning it to 'id' limits the list of time spent to the task after the update.

[eldy]

There is still suspicious change like addition of the old_taskid param when we should have it when we retreive the timespent to edit.
Can you confirmthat trouble is
Pb 1) Pb when you removed a user ? but wich one ?
Pb 2) Hiding some columns generate lose of data hidden if we edit line ?

[mdeweerd]

There is still suspicious change like addition of the old_taskid param when we should have it when we retreive the timespent to edit.

Can you confirmthat trouble is Pb

Pb 1) Pb when you removed a user ? 

& Pb 2) Hiding some columns generate lose of data hidden if we edit line ?

The initial problem occurs when I remove the user column from the timespent view. In this case the user is not part of the POST submission.
So it is not a specific user that is removed from the system, but simply the column that is not shown.
So yes, hiding columns results in loosing data (it's not added to hidden fields).

Pb 3) Timespend delete/add in stead of update.

In the code there is a test to check if the task to which the timespent entry is attached has changed or not. If that is the case, then there was a delete/add, if not an update;
However in the overview I use, this taskid was not present in the post, so empty. And therefore considered changed.

When I added the original task id as a "POST id" value, then after the update this resulted in a view specific to the task, and not all the tasks of the project. So I added old_task_id instead to return to the expected view after the update.
Generally, I would fetch the current record from the database and compare the submitted values to that information, not to information about the old data in a POST.
However the original code already retrieved it from a POST value so I kept it that way. It is not clear what is allowed/the approach to fetch the timespent record, read the fk_element (related task) from the timespent entry. The code gets the task first and then gets the timespent item. But based on the way it is done we need the timespent item first before finding the original task.

Pb 4) When changing the hour/time of a timespent entry, this was not taken into account.