G1 - HR Module#1898
Conversation
|
Congratulations for making your first Pull Request at Fusion!! 🎉 Someone from our team will review it soon. |
vikrantwiz02
left a comment
vikrantwiz02
left a comment
There was a problem hiding this comment.
Code Review — G1 HR Module (Backend)
Good architectural foundation — services/selectors/workflow layers are cleanly separated, BaseForm as an abstract base is the right pattern, and the custom permission class is well-structured. There are blockers to resolve before this can merge: active merge conflict on target branch, binary/scratch files that should not be committed, a silent breaking change on app_name, and an N+1 in the permission check. Details inline.
| designation_name = getattr(hold.designation, "name", None) | ||
| if not designation_name: | ||
| continue | ||
| access = ModuleAccess.objects.filter( |
There was a problem hiding this comment.
N+1 query per designation. For every designation the user holds, a separate ModuleAccess.objects.filter(...) is fired. On a user with many designations this hits the DB once per row. Collapse into a single query:
designation_names = [
getattr(h.designation, 'name', '').strip()
for h in working_designations
if getattr(h.designation, 'name', None)
]
return ModuleAccess.objects.filter(
designation__in=designation_names,
hr=True,
).exists()If case-insensitivity is required, normalise to lowercase before the filter instead of using __iexact per-row.
|
|
||
|
|
||
| app_name = 'hr2' | ||
| app_name = 'hr2_refactored' |
There was a problem hiding this comment.
Breaking change: app_name renamed from 'hr2' to 'hr2_refactored'. Any reverse('hr2:...') call in views, templates, or tests will raise NoReverseMatch after this merges. Either keep it as 'hr2', or do a full grep across the repo and update every reverse call before renaming.
| dependencies = [ | ||
| ('hr2', '0001_initial'), | ||
| ] | ||
|
|
There was a problem hiding this comment.
Migration history rewritten — will break existing deployments. The old 0002_auto_20241020_1126.py is deleted and a new 0002_add_leave_pdf_file.py put in its place. Any database that already ran the original 0002 will fail with InconsistentMigrationHistory on the next migrate. If this was applied anywhere (CI, staging), squash or merge migrations rather than replacing them.
| @@ -0,0 +1,209 @@ | |||
| import os | |||
There was a problem hiding this comment.
Dev-only scripts should not be committed to the repo. create_demo_users.py, create_hr_roles.py, create_superuser.py, scratch_create_users.py, scratch_delete_ltc.py, scratch_insert.py, and scratch_migrate.py are all local one-off scripts. Any persistent setup logic should be a proper manage.py management command; everything else should be deleted and added to .gitignore.
| pfNo = models.IntegerField(null=True) | ||
| submissionDate = models.DateField(blank=True, null=True) | ||
| approved = models.BooleanField(null=True) | ||
| approvedDate = models.DateField(auto_now_add=True, null=True) |
There was a problem hiding this comment.
auto_now_add=True makes approvedDate non-editable. Once set on creation, it cannot be overridden via the ORM (e.g. for backfills or corrections). Since this is an approval date (set during a workflow step, not on row creation), use default=None, blank=True, null=True and set it explicitly in the workflow:
approvedDate = models.DateField(blank=True, null=True, default=None)| WF_HR_APPROVED = "hr_approved" | ||
| WF_HR_REJECTED = "hr_rejected" | ||
|
|
||
| TERMINAL_STATUSES = frozenset( |
There was a problem hiding this comment.
String constants across multiple files risk silent typos. These WF_* constants are defined here but compared against raw strings in the serializer and views. A misspelling anywhere is a silent mismatch. Consider a TextChoices class or a shared LeaveWorkflowStatus enum so mismatches raise AttributeError at import time rather than silently failing at runtime.
|
Additional issues: Binary archive Merge conflict with target branch |
This PR is showing merge conflicts because the target branch has newer changes in some HR module files (
models.py,urls.py,views.py) that overlap with the changes in this PR, so Git is unable to automatically decide which version to keep.In this PR, the old
views.pyandurls.pyfiles outside theapifolder were intentionally removed because we were instructed to shift the implementation into theapistructure. The newer implementation is now inside theapifolder, so the old files are no longer required.To resolve the conflicts and allow automatic merge:
views.pyandurls.pyfiles from the target branch (hr-eis-v1) that are outside theapifoldermodels.pyAfter that, the PR should merge normally without issues.