Conversation
Anxo82
force-pushed
the
18.0-add-hr_collective_agreement
branch
from
36dd8d6 to
4ae9a5d
Compare
NL66278
left a comment
NL66278
left a comment
There was a problem hiding this comment.
👍 Thank you for your contribution. Just purge copyright lines from the init files and add some help or explanation and it will be good to go.
Anxo82
force-pushed
the
18.0-add-hr_collective_agreement
branch
2 times, most recently
from
a93336b to
3f34e24
Compare
NL66278
left a comment
NL66278
left a comment
There was a problem hiding this comment.
👍 Actually I am not quite sure whether the domain on the partner_id field is still needed with _check_company_auto, but it will not hurt anyway. Thanks again!
Jaimermaccione
left a comment
Jaimermaccione
left a comment
There was a problem hiding this comment.
The module has been reviewed functionally in a local environment. It works as expected and no issues were found in normal usage. Below are a few improvement ideas mainly related to usability and basic access control.
1. Unified search
Current situation
Users need to choose whether they want to search by code or name.
Proposal
Use a single search that checks both fields at the same time.
Value
Faster and more comfortable to use, especially when there are many agreements.
2. State management
Current situation
The state field is shown as a regular selection field inside the form.
Proposal
Move it to the header and show it as a status bar.
Extra note
In the current Spanish and Catalan translations, the state field is translated as “province”. It should simply be “state”.
Value
Makes it easier to understand the agreement status at a glance and follows the usual Odoo patterns.
3. Access rights
Current situation
Any internal user can create or edit collective agreements.
Proposal
Limit creation and editing to HR admin users, keeping read access for all internal users.
Optional
Adding separate configuration menus (scope, official publication) would be nice, but if that’s too much, the access rights change alone already helps a lot.
Value
Prevents accidental changes and ensures only users with the right level of access can modify this data.
Conclusion
Functionally, the module is fine. These are small improvements that would make it clearer and safer to use.
Anxo82
force-pushed
the
18.0-add-hr_collective_agreement
branch
from
3f34e24 to
77ab40e
Compare
|
@Jaimermaccione All requested changes have been applied. Could you please review again? |
Jaimermaccione
left a comment
Jaimermaccione
left a comment
There was a problem hiding this comment.
@Anxo82 The module has been functionally reviewed in a local environment and tested according to the requested changes.
Verified points:
- Unified search: search by code and name works correctly.
- State management: the state is shown as a status bar and behaves as expected.
- Access rights: creation and editing are correctly restricted by permissions.
- Automatic state updates: state changes based on publication date and expiration date work correctly.
- Translations: the “state” translation issue has been fixed.
All tested scenarios behave as expected.
Conclusion
LGTM. Good job — the module is in a very solid shape.
|
This PR has the |
Anxo82
force-pushed
the
18.0-add-hr_collective_agreement
branch
2 times, most recently
from
28083b2 to
96db0af
Compare
| if expired: | ||
| raise ValidationError( | ||
| _( | ||
| "The collective agreement cannot be activated because its " |
There was a problem hiding this comment.
If more than one register fails, how do you know which one is failing?
| def action_finish(self): | ||
| invalid = self.filtered(lambda r: r.state != "active") | ||
| if invalid: | ||
| raise ValidationError(_("Only active agreements can be finished.")) |
There was a problem hiding this comment.
If more than one register fails, how do you know which one is failing?
| self.write({"state": "finished"}) | ||
|
|
||
| def action_cancel(self): | ||
| self.write({"state": "cancelled"}) |
There was a problem hiding this comment.
Here, states are not checked, or can anyone cancel from anywhere?
Anxo82
force-pushed
the
18.0-add-hr_collective_agreement
branch
from
96db0af to
1ccf9c0
Compare
Jaimermaccione
left a comment
Jaimermaccione
left a comment
There was a problem hiding this comment.
@Anxo82 Hi Anxo,
I have functionally reviewed the new mass actions for cancelling and/or finishing collective agreements.
From a functional perspective, both actions work correctly and behave as expected depending on the agreement state. Good job on that.
1. README clarification (mass actions behavior)
I would suggest refining the README to better specify how the mass actions behave depending on the agreement state.
Instead of:
Additionally, multiple agreements can be selected from the list view and
processed simultaneously using the action menu.
I would propose something more explicit, such as:
Additionally, multiple agreements can be selected from the list view and
processed simultaneously using the action menu.From this action menu, users can:
- Cancel agreements that are in Draft or Active state.
- Finish agreements that are in Active state only.
Please review this wording in case anything does not exactly match the implemented behavior.
As a small UX improvement suggestion:
currently, when selecting agreements in mixed states, the action is shown and a warning is displayed if some records are not eligible, which is perfectly fine.
As a possible enhancement, it might be worth considering enabling or displaying mass actions only when all selected agreements are in compatible states (or showing them disabled with a tooltip). This would make the behavior more predictable and prevent invalid selections upfront.
That said, since the current behavior is already correct, this would only make sense if it can be implemented as a very lightweight change; otherwise, the existing approach is perfectly acceptable.
2. Duplication behavior (small usability improvement)
Apologies if this goes slightly beyond the current scope.
While testing the feature, I needed to duplicate some agreements and noticed that duplication is currently blocked because the code field cannot be repeated (which is correct).
However, it might improve usability if, when duplicating a record, the code is not copied to the new record. This would allow users to reuse most of the configuration values while still being forced to define a new unique code.
Could you please review whether it would be feasible to override the duplication behavior so that code is cleared on copy?
Let me know your thoughts.
Thanks!
5 participants
@HaraldPanten @Jaimermaccione