fix: apply company filter in leave period in leave control panel doctype#4234
Open
anwarpatelnoori wants to merge 3 commits intofrappe:developfrom
Open
fix: apply company filter in leave period in leave control panel doctype#4234anwarpatelnoori wants to merge 3 commits intofrappe:developfrom
anwarpatelnoori wants to merge 3 commits intofrappe:developfrom
Conversation
anwarpatelnoori
requested review from
asmitahase and
ruchamahabal
as code owners
Contributor
WalkthroughThe Leave Control Panel changes make company the propagated context: on setup the form uses the current company value, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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. Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hrms/hr/doctype/leave_control_panel/leave_control_panel.json (1)
1-3:⚠️ Potential issue | 🟡 MinorFix Prettier formatting.
Same as the JS file - run
prettier --writeto resolve the formatting check failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.json` around lines 1 - 3, The JSON file leave_control_panel.json (and the corresponding JS file leave_control_panel.js) fails formatting checks; run Prettier to fix it by executing prettier --write on these files (or your repo) to normalize indentation and trailing commas so the JSON keys/arrays match Prettier output; commit the reformatted files (ensure leave_control_panel.json valid JSON after formatting).hrms/hr/doctype/leave_control_panel/leave_control_panel.js (1)
1-4:⚠️ Potential issue | 🟡 MinorFix Prettier formatting to pass CI.
The pipeline is failing due to a Prettier formatting check. Run
prettier --writeon this file to resolve the formatting issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js` around lines 1 - 4, Run Prettier on the JavaScript file to fix formatting so CI passes: format leave_control_panel.js (the module that starts with frappe.ui.form.on("Leave Control Panel", { ... })) by running prettier --write against this file or applying your project's Prettier configuration, then commit the reformatted file so the Prettier check in the pipeline is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js`:
- Around line 38-44: The current frm.set_query for the "leave_period" field
replaces the earlier query and only filters by company, causing inactive periods
to surface; update the frm.set_query callback for "leave_period" to include both
filters (company: frm.doc.company and is_active: 1) so the dropdown only shows
active leave periods for the selected company (locate the set_query call for
"leave_period" in leave_control_panel.js and add the is_active filter to the
returned filters object).
---
Outside diff comments:
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js`:
- Around line 1-4: Run Prettier on the JavaScript file to fix formatting so CI
passes: format leave_control_panel.js (the module that starts with
frappe.ui.form.on("Leave Control Panel", { ... })) by running prettier --write
against this file or applying your project's Prettier configuration, then commit
the reformatted file so the Prettier check in the pipeline is satisfied.
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.json`:
- Around line 1-3: The JSON file leave_control_panel.json (and the corresponding
JS file leave_control_panel.js) fails formatting checks; run Prettier to fix it
by executing prettier --write on these files (or your repo) to normalize
indentation and trailing commas so the JSON keys/arrays match Prettier output;
commit the reformatted files (ensure leave_control_panel.json valid JSON after
formatting).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd072326-17b7-4683-a380-185927465db5
📒 Files selected for processing (2)
hrms/hr/doctype/leave_control_panel/leave_control_panel.jshrms/hr/doctype/leave_control_panel/leave_control_panel.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hrms/hr/doctype/leave_control_panel/leave_control_panel.js (1)
104-117:⚠️ Potential issue | 🟠 MajorTie the async leave-period response to the company that initiated it.
The callback currently reads
frm.doc.companywhen the promise resolves. If the user switches company twice before the first request returns, a slower response can still write the old company'sleave_periodinto the new company context. Returning the promise here also letscompany(frm)wait for the refresh.🐛 Proposed fix
set_leave_details(frm) { - frm.call("get_latest_leave_period").then((r) => { + const company = frm.doc.company; + if (!company) return Promise.resolve(); + + return frm.call("get_latest_leave_period").then((r) => { + if (frm.doc.company !== company) return; + frm.set_value({ dates_based_on: "Leave Period", from_date: frappe.datetime.get_today(), to_date: null, leave_period: r.message, carry_forward: 1, allocate_based_on_leave_policy: 1, leave_type: null, no_of_days: 0, leave_policy: null, - company: frm.doc.company, }); }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js` around lines 104 - 117, The async callback in set_leave_details uses frm.doc.company when the get_latest_leave_period promise resolves, causing a race where a stale company can be written; change set_leave_details so it captures the current company before calling frm.call (e.g., const currentCompany = frm.doc.company), pass/compare that captured value in the then handler and only call frm.set_value when it still matches, and return the frm.call Promise so callers like company(frm) can await the refresh; reference: set_leave_details, get_latest_leave_period, frm.set_value, company(frm).
♻️ Duplicate comments (1)
hrms/hr/doctype/leave_control_panel/leave_control_panel.js (1)
38-44:⚠️ Potential issue | 🟠 MajorKeep a single
leave_periodquery definition.This second
frm.set_query("leave_period", ...)diverges from the one at Line 173 and dropsis_active: 1, so inactive periods reappear after a company change and the final filter depends on which callback ran last.🐛 Proposed fix
- frm.set_query("leave_period", function () { - return { - filters: { - company: frm.doc.company, - }, - }; - });// Prefer defining `leave_period` once in `set_query()`: frm.set_query("leave_period", function () { return { filters: { is_active: 1, company: frm.doc.company, }, }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js` around lines 38 - 44, There are two conflicting frm.set_query("leave_period", ...) definitions causing the is_active: 1 filter to be dropped depending on which callback runs last; remove the duplicate and keep a single frm.set_query("leave_period", ...) that returns filters including both is_active: 1 and company: frm.doc.company so inactive periods are always excluded (update the existing set_query call used for leave_period rather than adding a second one).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js`:
- Around line 29-45: The company handler runs frm.trigger("get_employees")
before the async server call in set_leave_details finishes, causing stale
leave_period; modify set_leave_details(frm) to return the promise from its
frm.call (so it resolves after get_latest_leave_period completes) and make the
company function async so it awaits frm.trigger("set_leave_details") when
frm.doc.dates_based_on === "Leave Period"; keep the department/leave_period
frm.set_query calls intact before awaiting and only call
frm.trigger("get_employees") after the await to ensure employees load with the
updated leave_period.
---
Outside diff comments:
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js`:
- Around line 104-117: The async callback in set_leave_details uses
frm.doc.company when the get_latest_leave_period promise resolves, causing a
race where a stale company can be written; change set_leave_details so it
captures the current company before calling frm.call (e.g., const currentCompany
= frm.doc.company), pass/compare that captured value in the then handler and
only call frm.set_value when it still matches, and return the frm.call Promise
so callers like company(frm) can await the refresh; reference:
set_leave_details, get_latest_leave_period, frm.set_value, company(frm).
---
Duplicate comments:
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js`:
- Around line 38-44: There are two conflicting frm.set_query("leave_period",
...) definitions causing the is_active: 1 filter to be dropped depending on
which callback runs last; remove the duplicate and keep a single
frm.set_query("leave_period", ...) that returns filters including both
is_active: 1 and company: frm.doc.company so inactive periods are always
excluded (update the existing set_query call used for leave_period rather than
adding a second one).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3af591d2-5f3b-46b1-81c0-823009f4b4e5
📒 Files selected for processing (1)
hrms/hr/doctype/leave_control_panel/leave_control_panel.js
Comment on lines
29
to
45
| company: function (frm) { | ||
| if (frm.doc.company) { | ||
| frm.set_query("department", function () { | ||
| return { | ||
| filters: { | ||
| company: frm.doc.company, | ||
| }, | ||
| }; | ||
| }); | ||
| } | ||
| frm.trigger("set_leave_details"); | ||
| frm.set_query("department", function () { | ||
| return { | ||
| filters: { | ||
| company: frm.doc.company, | ||
| }, | ||
| }; | ||
| }); | ||
| frm.set_query("leave_period", function () { | ||
| return { | ||
| filters: { | ||
| company: frm.doc.company, | ||
| }, | ||
| }; | ||
| }); | ||
| frm.trigger("get_employees"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the leave_control_panel.js file
find . -name "leave_control_panel.js" -type f
Repository: frappe/hrms
Length of output: 115
🏁 Script executed:
# Read the full file to understand the implementation
cat -n hrms/hr/doctype/leave_control_panel/leave_control_panel.js
Repository: frappe/hrms
Length of output: 5963
Wait for leave details to sync when company changes.
set_leave_details() initiates a server call on line 30, but get_employees() on line 38 runs immediately. When switching companies in "Leave Period" mode, the employee load receives the previous company's stale leave_period value. Make set_leave_details() return its promise and await it:
set_leave_details(frm) {
return frm.call("get_latest_leave_period").then((r) => {
frm.set_value({
dates_based_on: "Leave Period",
from_date: frappe.datetime.get_today(),
to_date: null,
leave_period: r.message,
carry_forward: 1,
allocate_based_on_leave_policy: 1,
leave_type: null,
no_of_days: 0,
leave_policy: null,
company: frm.doc.company,
});
});
},
Then in the company function:
company: async function (frm) {
frm.set_query("department", function () {
return {
filters: {
company: frm.doc.company,
},
};
});
if (frm.doc.dates_based_on === "Leave Period") {
await frm.trigger("set_leave_details");
}
frm.trigger("get_employees");
},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js` around lines 29 -
45, The company handler runs frm.trigger("get_employees") before the async
server call in set_leave_details finishes, causing stale leave_period; modify
set_leave_details(frm) to return the promise from its frm.call (so it resolves
after get_latest_leave_period completes) and make the company function async so
it awaits frm.trigger("set_leave_details") when frm.doc.dates_based_on ===
"Leave Period"; keep the department/leave_period frm.set_query calls intact
before awaiting and only call frm.trigger("get_employees") after the await to
ensure employees load with the updated leave_period.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hrms/hr/doctype/leave_control_panel/leave_control_panel.js (1)
29-39:⚠️ Potential issue | 🟠 MajorRace condition:
get_employeesruns beforeset_leave_detailscompletes.
set_leave_details(line 30) makes an async server call viafrm.call("get_latest_leave_period"), butget_employees(line 38) executes immediately without waiting. When switching companies in "Leave Period" mode, employees may load with staleleave_period/date values.🐛 Proposed fix: await set_leave_details before fetching employees
- company: function (frm) { - frm.trigger("set_leave_details"); + company: async function (frm) { frm.set_query("department", function () { return { filters: { company: frm.doc.company, }, }; }); + frm.set_query("leave_period", function () { + return { + filters: { + is_active: 1, + company: frm.doc.company, + }, + }; + }); + if (frm.doc.dates_based_on === "Leave Period") { + await frm.trigger("set_leave_details"); + } frm.trigger("get_employees"); },And modify
set_leave_detailsto return the promise:set_leave_details(frm) { - frm.call("get_latest_leave_period").then((r) => { + return frm.call("get_latest_leave_period").then((r) => { frm.set_value({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js` around lines 29 - 39, The company handler calls frm.trigger("set_leave_details") but immediately calls frm.trigger("get_employees"), causing a race; change the company function to wait for set_leave_details to complete (await frm.trigger("set_leave_details") or use .then) before calling frm.trigger("get_employees"), and update set_leave_details to return the promise from its async call (the frm.call("get_latest_leave_period") promise) so that frm.trigger resolves only after the server call finishes; this ensures get_employees runs after set_leave_details (and its get_latest_leave_period) completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hrms/hr/doctype/leave_control_panel/leave_control_panel.js`:
- Around line 29-39: The company handler calls frm.trigger("set_leave_details")
but immediately calls frm.trigger("get_employees"), causing a race; change the
company function to wait for set_leave_details to complete (await
frm.trigger("set_leave_details") or use .then) before calling
frm.trigger("get_employees"), and update set_leave_details to return the promise
from its async call (the frm.call("get_latest_leave_period") promise) so that
frm.trigger resolves only after the server call finishes; this ensures
get_employees runs after set_leave_details (and its get_latest_leave_period)
completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1a5c586-2bb6-493b-8eb1-05976514e111
📒 Files selected for processing (1)
hrms/hr/doctype/leave_control_panel/leave_control_panel.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4233
Information about bug
Pre-requisites
Setup multi-company environment
Create Leave Period for each company.
Steps to Reproduce
In Leave Control Panel By default:
Default Company and its latest Leave Period will be auto-selected.
Employees of that company will be rendered in the table.
Example:
Now select the Leave Period of another company.
Example:
Bug
The employee table still shows employees of the default company (Demo Company).
Employees belonging to the selected company (Test Company) are not rendered.
If employees are selected and Leave Policy is assigned:
set_leave_details, usefrm.doc.companyinstead of the default company.Before
Leave.Control.Panel.Bug.1.mp4
After
Leave.Control.Bug.Fixed.mp4
Summary by CodeRabbit