[ISSUE-3709] Fix dragging of tasks in CW that have predecessors #449
Conversation
zeryab-topleft
force-pushed
the
3709-fix-dragging-of-tasks-in-cw-that-have-predecessors
branch
2 times, most recently
from
5b1a5c9 to
daefd87
Compare
|
|
||
| predecessor_reset_fields = { | ||
| 'ticket_predecessor': None, | ||
| 'required_date_utc': None, |
There was a problem hiding this comment.
Does the required_date_utc NEED to be set as none for this as well? I don't think it does, and that creates the problem of it also removing this and never adding it back. Check to make sure we don't have to null this field.
There was a problem hiding this comment.
Oh, also this appears to happen when even the start date is updated. So if a user updates a start date, doesn't this erase the required date and then not add it back?
There was a problem hiding this comment.
Does the required_date_utc NEED to be set as none for this as well? I don't think it does, and that creates the problem of it also removing this and never adding it back. Check to make sure we don't have to null this field.
Yes, I have added it on purpose if we don't remove the required_date_utc or estimated_start_date the same dependency error occurs.
There was a problem hiding this comment.
Oh, also this appears to happen when even the start date is updated. So if a user updates a start date, doesn't this erase the required date and then not add it back?
Yes, if a ticket has predecessor and user change either start or end date we need to set both to null and then set the date. Otherwise the dependecy error occurs
| predecessor_reset = False | ||
| except ConnectWiseAPIError as e: | ||
| error_message = \ | ||
| "Failed to reset predecessor fields for record " + \ |
There was a problem hiding this comment.
This is a good internal logging message, but it means nothing for the user. They don't need to know and probably wont understand why this step is important, and it doesn't give them any indication of what they need to do. It would be better to just tell them that the update request failed, and to refresh and try again.
| updated_record = client.update_ticket(record, api_fields) | ||
| except ConnectWiseAPIError as e: | ||
| error_message = \ | ||
| f"Failed to update record {record.id} with changed fields." |
There was a problem hiding this comment.
Also too generic for the user, and doesn't help them understand what to do next. Fix similar to the comment on line 2948
djconnectwise/sync.py
Outdated
| f"Failed to update record {record.id} with changed fields." | ||
| if not predecessor_reset: | ||
| error_message = ( | ||
| f"Failed to update record {record.id} after " + |
There was a problem hiding this comment.
This error message is also not great for the user. We need to be very clear that something has gone wrong and the predecessor is gone and they need to fix it. Something like:
"An error occurred while updating record {id} and the predecessor has been removed from the ticket. You must re-add the predecessor manually to the ticket."
Its also a good idea to do this in 3 operations instead of 2. While 3 does give it more opportunities to fail, if the first two succeed, then the data they wanted to change is still changed, they just need to fix the predecessor. If the second one fails but the third succeeds, then we can at least set it back to what it was before they tried. If the first fails, we just show a message to try again.
There was a problem hiding this comment.
I have updated the error message, also if we do it using 3 api calls it still fails with dependency error. So I have to do it with 2 where I need to set the start and end Date along with predecessor at time.
ssarwar-topleft
force-pushed
the
3709-fix-dragging-of-tasks-in-cw-that-have-predecessors
branch
2 times, most recently
from
6904af0 to
3d4169c
Compare
zeryab-topleft
force-pushed
the
3709-fix-dragging-of-tasks-in-cw-that-have-predecessors
branch
from
3d4169c to
d6a66b5
Compare
No description provided.