Hotfix zmskvr set workstation select resolve references from 3 to 2

[ThomasAFink]

Pull Request Checklist (Feature Branch to next):

• Ich habe die neuesten Änderungen aus dem next Branch in meinen Feature-Branch gemergt.
• Das Code-Review wurde abgeschlossen.
• Fachliche Tests wurden durchgeführt und sind abgeschlossen.

Summary by CodeRabbit

• Chores
  • Reduced how deeply related data is resolved when selecting a workstation and ensured cluster references are resolved at least one level.
• Tests
  • Updated tests and fixtures to reflect the adjusted reference resolution behavior.

[coderabbitai]

Walkthrough

Reduced reference-resolution depth: WorkstationSelect.php now requests one fewer reference level (3 → 2) when reading workstation data; Department::readResolvedReferences guarantees at least 1 for cluster resolution. Corresponding test fixtures and expectations were updated.

Changes

Cohort / File(s)	Summary
Workstation select change zmsadmin/src/Zmsadmin/WorkstationSelect.php	Lowered resolveReferences from 3 to 2 in readGetResult call inside readResponse.
Department cluster resolution zmsdb/src/Zmsdb/Department.php	Changed cluster read call to pass max(1, resolveReferences - 1) instead of resolveReferences - 1, ensuring at least 1 when resolving clusters.
Tests / Fixtures zmsadmin/tests/Zmsadmin/WorkstationSelectTest.php	Updated multiple test cases to expect resolveReferences = 2 and switched fixtures from *_resolved3.json to corresponding *_resolved2.json variants across testRendering, testLoginFailed, testRenderingSelect, testRenderingSelectWithCustomRedirect, testRenderingSelectFailedValidation, and testRenderingSelectAppointmentsOnlyWithCluster.

Sequence Diagram(s)

(No sequence diagram provided — changes are a parameter adjustment and a small safeguard, not a control-flow modification.)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• fix(ZMSKVR): hotfix-zmskvr-set-workstation-select-resolve-references-from-3-to-2 #1619: Same change to workstation resolveReferences (3 → 2) and matching test fixture updates.

Poem

🐇 A nibble at the reference, I hop from three to two,

The clusters keep their footing, at least one comes through,
Tests updated, fixtures bright, a tidy little tune,
I thump my feet, a tiny cheer — a change that hums in tune.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Hotfix zmskvr set workstation select resolve references from 3 to 2" is directly and fully related to the main change in the changeset. The title accurately describes the primary technical modification: lowering the resolveReferences parameter from 3 to 2 in the workstation selection logic. The title is specific and clear, identifying both the affected component (workstation select) and the exact parameter change, allowing teammates to understand the core change from the title alone. While the PR objectives indicate this change may introduce a functional issue with cluster location display, that concern is separate from whether the title accurately describes what was technically changed in the code.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch hotfix-zmskvr-set-workstation-select-resolve-references-from-3-to-2

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between caf660f and 35b9f8e.

⛔ Files ignored due to path filters (1)

• zmscitizenview/package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (1)

• zmsdb/src/Zmsdb/Department.php (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*

⚙️ CodeRabbit configuration file

**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:

Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.

General rules

1. Follow standard conventions.
2. Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
3. Boy scout rule. Leave the campground cleaner than you found it.
4. Always find root cause. Always look for the root cause of a problem.

Design rules

1. Keep configurable data at high levels.
2. Prefer polymorphism to if/else or switch/case.
3. Separate multi-threading code.
4. Prevent over-configurability.
5. Use dependency injection.
6. Follow Law of Demeter. A class should know only its direct dependencies.

Understandability tips

1. Be consistent. If you do something a certain way, do all similar things in the same way.
2. Use explanatory variables.
3. Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
4. Prefer dedicated value objects to primitive type.
5. Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
6. Avoid negative conditionals.

Names rules

1. Choose descriptive and unambiguous names.
2. Make meaningful distinction.
3. Use pronounceable names.
4. Use searchable names.
5. Replace magic numbers with named constants.
6. Avoid encodings. Don't append prefixes or type information.

Functions rules

1. Small.
2. Do one thing.
3. Use descriptive names.
4. Prefer fewer arguments.
5. Have no side effects.
6. Don't use flag arguments. Split method into several independent methods that can b...

Files:

• zmsdb/src/Zmsdb/Department.php

**/*.php

⚙️ CodeRabbit configuration file

**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:

1. For error handling: Use the proper Monolog logging framework with error levels
2. For application info logs: Use the proper Monolog logging framework with info levels
3. For debugging: Use a dedicated debug logger or remove debug statements
4. For CLI output: Use a CLI output handler or symfony/console
5. The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY

Example replacement:

// Instead of:
error_log("Import failed - " . $e->getMessage());

// Use:
$log->error("Import failed", ['error' => $e->getMessage()]);

Flag specific logging violations:

1. error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
2. Any debug output that should use proper Monolog logging
3. Debug constants like DEBUG = true
4. Debug logging that should be removed in production
5. Commented debug code that should be cleaned up

Example replacements:

// Instead of:
error_log("Error occurred");
var_dump($data);
die('debug point');

// Use:
$log->error("Error occurred", ['context' => 'processing']);
$log->debug('Data dump', ['data' => $data]);
// Remove die() statements entirely

Exception handling should use proper logging:

// Instead of:
try {
    $result = $this->process();
} catch (Exception $e) {
    error_log("Processing failed: " . $e->getMessage());
}

// Use:
try {
    $result = $this->process();
} catch (Exception $e) {
    $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]);
}
```...

Files:

• zmsdb/src/Zmsdb/Department.php

🧬 Code graph analysis (1)

zmsdb/src/Zmsdb/Department.php (1)

zmsadmin/js/page/overallCalendar/overallCalendar.js (1)

• max (580-580)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: call-php-unit-tests / zmsdb-test

🔇 Additional comments (1)

zmsdb/src/Zmsdb/Department.php (1)

56-64: Now I can generate the final rewritten review comment with confidence:

Review the root cause: zmsadmin WorkstationSelect depth has been reduced from 3 to 2, causing insufficient scope resolution depth downstream.

The max(1, ...) guard on line 62 prevents zero/negative depths but doesn't address the core issue. With zmsadmin calling at depth 2 (as shown in the test fixtures):

• Department scopes resolve at depth 1 (line 57)
• Department clusters resolve at depth 1 (line 62: max(1, 2-1) = 1)
• Cluster scopes resolve at depth 1 (Cluster::readResolvedReferences line 60 passes depth directly)
• Scope data at depth 1 contains only id and $ref, lacking contact names needed for cluster location display

Before the PR, zmsadmin called with depth 3, which cascaded as: Department → depth 2 → Cluster → depth 2 for scopes (sufficient for names).

Required action: Verify that zmsadmin/src/Zmsadmin/WorkstationSelect.php has been reverted to ['resolveReferences' => 3] in this PR. If it remains at 2, either:

1. Revert it to 3, or
2. Add max(2, ...) to line 62 to ensure cluster scopes receive at least depth 2 for name resolution

Additionally, consider applying the same guard to line 57 (Department scopes) for consistency: max(1, $resolveReferences - 1).

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ThomasAFink]

@coderabbitai problem is now the cluster location names no longer show up in the selection list in zmsadmin. They show up in the list as deleted locations.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

zmsadmin/src/Zmsadmin/WorkstationSelect.php (1)

25-25: Accept change (resolveReferences 3 → 2); document rationale and extract magic number.

• Evidence: WorkstationSelectTest and codebase searches show resolveReferences => 2 is expected and sufficient (zmsadmin/src/Zmsadmin/WorkstationSelect.php:25; zmsadmin/tests/Zmsadmin/WorkstationSelectTest.php).
• Downstream: LoginForm::writeWorkstationUpdate only uses useraccount-level fields and the template receives the workstation object (zmsadmin/src/Zmsadmin/Helper/LoginForm.php; zmsadmin/templates/page/workstationSelect.twig).
• Action items (non-blocking): add a one-line comment or PR note stating why resolution was reduced (e.g. performance/over‑fetching) and replace the literal 2 with a named constant to avoid the magic number.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f21d0f9 and caf660f.

📒 Files selected for processing (2)

• zmsadmin/src/Zmsadmin/WorkstationSelect.php (1 hunks)
• zmsadmin/tests/Zmsadmin/WorkstationSelectTest.php (6 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*

⚙️ CodeRabbit configuration file

**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:

Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.

General rules

1. Follow standard conventions.
2. Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
3. Boy scout rule. Leave the campground cleaner than you found it.
4. Always find root cause. Always look for the root cause of a problem.

Design rules

1. Keep configurable data at high levels.
2. Prefer polymorphism to if/else or switch/case.
3. Separate multi-threading code.
4. Prevent over-configurability.
5. Use dependency injection.
6. Follow Law of Demeter. A class should know only its direct dependencies.

Understandability tips

1. Be consistent. If you do something a certain way, do all similar things in the same way.
2. Use explanatory variables.
3. Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
4. Prefer dedicated value objects to primitive type.
5. Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
6. Avoid negative conditionals.

Names rules

1. Choose descriptive and unambiguous names.
2. Make meaningful distinction.
3. Use pronounceable names.
4. Use searchable names.
5. Replace magic numbers with named constants.
6. Avoid encodings. Don't append prefixes or type information.

Functions rules

1. Small.
2. Do one thing.
3. Use descriptive names.
4. Prefer fewer arguments.
5. Have no side effects.
6. Don't use flag arguments. Split method into several independent methods that can b...

Files:

• zmsadmin/src/Zmsadmin/WorkstationSelect.php
• zmsadmin/tests/Zmsadmin/WorkstationSelectTest.php

**/*.php

⚙️ CodeRabbit configuration file

**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:

1. For error handling: Use the proper Monolog logging framework with error levels
2. For application info logs: Use the proper Monolog logging framework with info levels
3. For debugging: Use a dedicated debug logger or remove debug statements
4. For CLI output: Use a CLI output handler or symfony/console
5. The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY

Example replacement:

// Instead of:
error_log("Import failed - " . $e->getMessage());

// Use:
$log->error("Import failed", ['error' => $e->getMessage()]);

Flag specific logging violations:

1. error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
2. Any debug output that should use proper Monolog logging
3. Debug constants like DEBUG = true
4. Debug logging that should be removed in production
5. Commented debug code that should be cleaned up

Example replacements:

// Instead of:
error_log("Error occurred");
var_dump($data);
die('debug point');

// Use:
$log->error("Error occurred", ['context' => 'processing']);
$log->debug('Data dump', ['data' => $data]);
// Remove die() statements entirely

Exception handling should use proper logging:

// Instead of:
try {
    $result = $this->process();
} catch (Exception $e) {
    error_log("Processing failed: " . $e->getMessage());
}

// Use:
try {
    $result = $this->process();
} catch (Exception $e) {
    $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]);
}
```...

Files:

• zmsadmin/src/Zmsadmin/WorkstationSelect.php
• zmsadmin/tests/Zmsadmin/WorkstationSelectTest.php

🧬 Code graph analysis (2)

zmsadmin/src/Zmsadmin/WorkstationSelect.php (1)

zmsclient/src/Zmsclient/Http.php (1)

• readGetResult (175-188)

zmsadmin/tests/Zmsadmin/WorkstationSelectTest.php (1)

zmsapi/tests/Zmsapi/Base.php (1)

• readFixture (31-38)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: call-php-unit-tests / zmsdb-test

🔇 Additional comments (2)

zmsadmin/tests/Zmsadmin/WorkstationSelectTest.php (2)

20-21: LGTM - Tests properly updated to match implementation change.

All test expectations have been correctly updated to reflect the reduced resolveReferences parameter (from 3 to 2) and corresponding fixture files have been changed from resolved3.json to resolved2.json. This ensures that tests validate the actual behavior of the updated code.

Also applies to: 38-38, 55-56, 61-61, 82-83, 88-88, 133-134, 139-139

---

110-111: No action needed – fixture file is correct.

The fixture GET_Workstation_Resolved2.json is a valid file in the repository. The naming difference from other tests is intentional: GET_Workstation_Resolved2.json (without "with_process") represents a basic resolved workstation, while GET_workstation_with_process_resolved2.json is used when process information is relevant. For a validation failure test, using the simpler fixture is appropriate.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff to see what was changed
gh pr diff 1620 --repo it-at-m/eappointment

Length of output: 5504

---

🏁 Script executed:

#!/bin/bash
# Search for files related to workstation selection and cluster
fd -e php -e js -e ts -e java | xargs rg -l "workstation.*select" -i | head -20

Length of output: 883

---

🏁 Script executed:

#!/bin/bash
# Search for references to resolve depth or reference levels
rg -n "resolveReferences|resolve.*depth|reference.*level" -A 3 -B 3 | head -100

Length of output: 10009

---

🏁 Script executed:

#!/bin/bash
# Search for cluster location related code
rg -n "cluster.*location|location.*cluster" -i -A 3 -B 3 | head -100

Length of output: 12070

---

🏁 Script executed:

#!/bin/bash
# Check the fixture file to see what data structure it contains
cat zmsadmin/tests/Zmsadmin/fixtures/GET_workstation_with_process_resolved2.json | jq '.' | head -200

Length of output: 7837

---

🏁 Script executed:

#!/bin/bash
# Also check GET_Workstation_Resolved2.json for comparison
cat zmsadmin/tests/Zmsadmin/fixtures/GET_Workstation_Resolved2.json | jq '.' | head -200

Length of output: 6089

---

🏁 Script executed:

#!/bin/bash
# Search for template files that render workstation/cluster selection
fd -e twig -e phtml | xargs rg -l "workstation.*select|cluster" -i | head -20

Length of output: 1435

---

🏁 Script executed:

#!/bin/bash
# Look at ClusterHelper to understand how clusters are processed
cat zmsadmin/src/Zmsadmin/Helper/ClusterHelper.php

Length of output: 2714

---

🏁 Script executed:

#!/bin/bash
# Search for how scopes/locations in clusters are displayed
rg -n "cluster.*scope|scope.*cluster" -A 3 -B 3 --type php | head -150

Length of output: 12689

---

🏁 Script executed:

#!/bin/bash
# Look at the workstation select template to see how clusters/scopes are rendered
cat zmsadmin/templates/page/workstationSelect.twig

Length of output: 595

---

🏁 Script executed:

#!/bin/bash
# Check if there's a specific template for displaying cluster options
find zmsadmin/templates -name "*.twig" -exec grep -l "cluster\|scope.*option\|option.*scope" {} \; | head -10

Length of output: 1102

---

🏁 Script executed:

#!/bin/bash
# Look at the workstation select form helper/template
fd selectform.twig | xargs cat

Length of output: 8738

---

🏁 Script executed:

#!/bin/bash
# Search for how the scope/cluster options are built in PHP
rg -n "option.*value.*scope|option.*cluster" --type php -A 2 -B 2 | head -100

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check the Department entity method that handles cluster scopes more carefully
rg -n "withOutClusterDuplicates|getScopeList" --type php -A 10 -B 2 | head -150

Length of output: 14006

---

@ThomasAFink I've identified the root cause of the issue. Let me explain what's happening:

Problem Analysis

When you changed resolveReferences from 3 to 2, you reduced the depth of entity reference resolution. This affects how cluster scope data is loaded:

With resolveReferences => 3:

• Level 1: Department
• Level 2: Department → Clusters
• Level 3: Department → Clusters → Scopes (fully resolved with names)

With resolveReferences => 2:

• Level 1: Department
• Level 2: Department → Clusters → Scopes (only ID and $ref)

Looking at the fixture file GET_Workstation_Resolved2.json, the cluster scopes only contain:

"scopes": [
    {
        "id": "140",
        "$ref": "/scope/140/"
    }
]

They're missing the provider.name and contact.name fields that the template needs to display location names.

Where the Problem Manifests

In zmsadmin/templates/block/workstation/selectform.twig (lines 46-50):

{% for cluster in department.clusters %}
    {% for scope in cluster.scopes %}
        {% set scopeOptionsGroupOptions = scopeOptionsGroupOptions|merge([{ "value": scope.id, "name": providername(scope) }]) %}
    {% endfor %}
{% endfor %}

The providername(scope) macro tries to extract the location name from the scope object, but since the scope is not fully resolved at level 2, it can't find the name data—resulting in the "deleted locations" appearance.

Solution

You'll need to revert to resolveReferences => 3 to ensure cluster location names are properly resolved and displayed. The original setting was correct for this use case.

If there was a specific performance reason for reducing to level 2, we'd need to investigate alternative approaches, such as:

• Pre-processing the department data to flatten cluster scopes
• Using a different API endpoint that returns the data in the required format
• Modifying the template to handle unresolved references differently

Would you like me to help investigate why this change was made in the first place, or would you prefer to simply revert it?