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

Issue #1447 customerticketzoom #1845

Open
wants to merge 7 commits into
base: rel-10_1
Choose a base branch
from

Conversation

stefanhaerter
Copy link
Contributor

@stefanhaerter stefanhaerter commented Jul 19, 2022

  1. I removed the code for displaying process and activity permanently, so from now on, if this information is wanted, the corresponding dynamic fields have to be used
  2. A reference bug caused broken activity information in the config at runtime

Closing #1447

@stefanhaerter stefanhaerter added the bug Something isn't working as intended label Jul 19, 2022
Copy link
Contributor

@bschmalhofer bschmalhofer left a comment

Choose a reason for hiding this comment

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

Looks fine. The removed code avoids the problem of inadvertently changing the data in the Kernel::Config instance.
I have one suggestion for further code cleanup. In line 1782 the variable $NextActivityDialogs is assigned a new value, unrelated to the previous value. In those cases I tend to give the variable a new name, declared with my.

@stefanhaerter
Copy link
Contributor Author

stefanhaerter commented Jul 20, 2022

Thank you for the good suggestion. I would make this change, but this seems to happen in L1795 also. I don't know if that makes much of a difference, but intuitively I would want to avoid initializing several variables which are not used later on - what do you think?

Maybe it could be a solution not to overwrite the existing variable but to actually filter the values with map?

@bschmalhofer
Copy link
Contributor

I haven't really looked closely at the code. The reuse of variables in different contexts is IMHO a red flag. If it occurs in several lines then it might make sense to rename or remove those variables. But of course only when there is no danger of unwanted side effects.

@stefanhaerter
Copy link
Contributor Author

I tried to reduce the assignments with my latest commit. Do you think this looks ok?

Copy link
Contributor

@svenoe svenoe left a comment

Choose a reason for hiding this comment

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

Please confirm that $ActivityData and $ActivityData->{ActivityDialog} are always hashrefs.
Please look for LayoutObject->Block(ProcessData) in AgentTicketZoom.

@stefanhaerter
Copy link
Contributor Author

I added checks with IsHashRefWithData on ActivityData and ActivityData->{ActivityDialog}.
AgentTicketZoom doesn't render a ProcessData block and apparently never did since the initial OTOBO commit.

@stefanhaerter stefanhaerter force-pushed the issue-#1447-customerticketzoom branch 2 times, most recently from 4b4b843 to 448c835 Compare October 19, 2023 15:51
@stefanhaerter stefanhaerter force-pushed the issue-#1447-customerticketzoom branch from 7792e9b to 9fe16c6 Compare January 26, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants