Fix: Fixed Astech & Medic Skill Generation #6875
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6875 +/- ##
============================================
+ Coverage 11.52% 11.57% +0.05%
- Complexity 6567 6612 +45
============================================
Files 1100 1100
Lines 140971 140973 +2
Branches 21845 21845
============================================
+ Hits 16243 16314 +71
+ Misses 123030 122962 -68
+ Partials 1698 1697 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue in the bulk hire dialog where the special handling for Astechs (and Medics) with non-zero experience levels was preventing their proper hiring. The changes remove the redundant check for isAssistant in the dialog, deprecate the isAssistant method in PersonnelRole, and adjust the personnel skill mapping in PersonUtility.
- Removed the isAssistant check in HireBulkPersonnelDialog to allow proper handling of Astechs.
- Deprecated the isAssistant method with an updated annotation and comment in PersonnelRole.
- Revised the skill mapping in PersonUtility to separately handle ASTECH, DOCTOR, and MEDIC roles.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| MekHQ/src/mekhq/gui/dialog/HireBulkPersonnelDialog.java | Removed isAssistant condition from the skill assignment logic. |
| MekHQ/src/mekhq/campaign/personnel/enums/PersonnelRole.java | Deprecated the isAssistant method with updated documentation. |
| MekHQ/src/mekhq/campaign/personnel/PersonUtility.java | Updated the mapping for ASTECH, DOCTOR, and MEDIC to reflect revised skill handling. |
Comments suppressed due to low confidence (2)
MekHQ/src/mekhq/gui/dialog/HireBulkPersonnelDialog.java:358
- Ensure that removal of the isAssistant condition aligns with all role eligibility rules; verify that no unintended roles are now included for skill assignment.
if ((useSkill) && (!selectedItem.getRole().isCivilian())) {
MekHQ/src/mekhq/campaign/personnel/PersonUtility.java:171
- Confirm that the new mapping for ASTECH correctly reflects the intended skill levels as per the updated business requirements.
Map.entry(ASTECH, List.of(S_ASTECH)),
| @@ -594,8 +594,9 @@ public boolean isMedicalStaff() { | |||
| } | |||
|
|
|||
| /** | |||
| * @return true if the person is an Astech or Medic, false otherwise. | |||
| * @return Unused | |||
There was a problem hiding this comment.
[nitpick] Consider updating the Javadoc for isAssistant to clearly explain its deprecation and link it to the revised skill handling logic.
| * @return Unused | |
| * This method is deprecated and will be removed in a future release. It is no longer used in the current | |
| * implementation of personnel role handling. | |
| * | |
| * <p>Instead of using this method, refer to the revised skill handling logic, which provides a more | |
| * comprehensive and flexible approach to managing personnel roles. For more details, see the documentation | |
| * for the {@code SkillAttribute} enum or consult the MekHQ user guide.</p> | |
| * | |
| * @return {@code true} if the character is assigned to an assistant role (e.g., Astech or Medic), {@code false} otherwise. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This should be @deprecated Unused
When we moved to ATOW skills in 50.05 Astechs and Medics were set up have the full array of experience levels, rather than just 0. However, there was a missed special handler that prevented the bulk hire dialog from hiring Astechs of experience levels greater than 0. This PR addresses that.