Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: convert dates to datetime before comparing in leave days calculation and fix half day edge case #30538

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

ruchamahabal
Copy link
Member

@ruchamahabal ruchamahabal commented Apr 1, 2022

  • TypeError while getting leaves with half-day for splitting ledger entries (in case of application spanning over 2 allocations or application created with carry-forwarded leave). Convert dates to datetime before comparing them in get_number_of_leave_days for leave days calculation as there are multiple sources from where this function gets called.
Traceback
Traceback (most recent call last):
  File "apps/frappe/frappe/desk/form/save.py", line 19, in savedocs
    doc.submit()
  File "apps/frappe/frappe/model/document.py", line 939, in submit
    return self._submit()
  File "apps/frappe/frappe/model/document.py", line 928, in _submit
    return self.save()
  File "apps/frappe/frappe/model/document.py", line 287, in save
    return self._save(*args, **kwargs)
  File "apps/frappe/frappe/model/document.py", line 341, in _save
    self.run_post_save_methods()
  File "apps/frappe/frappe/model/document.py", line 1007, in run_post_save_methods
    self.run_method("on_submit")
  File "apps/frappe/frappe/model/document.py", line 869, in run_method
    out = Document.hook(fn)(self, *args, **kwargs)
  File "apps/frappe/frappe/model/document.py", line 1161, in composer
    return composed(self, method, *args, **kwargs)
  File "apps/frappe/frappe/model/document.py", line 1144, in runner
    add_to_return_value(self, fn(self, *args, **kwargs))
  File "apps/frappe/frappe/model/document.py", line 866, in fn
    return method_object(*args, **kwargs)
  File "apps/erpnext/erpnext/hr/doctype/leave_application/leave_application.py", line 86, in on_submit
    self.create_leave_ledger_entry()
  File "apps/erpnext/erpnext/hr/doctype/leave_application/leave_application.py", line 467, in create_leave_ledger_entry
    self.create_ledger_entry_for_intermediate_allocation_expiry(expiry_date, submit, lwp)
  File "apps/erpnext/erpnext/hr/doctype/leave_application/leave_application.py", line 539, in create_ledger_entry_for_intermediate_allocation_expiry
    leaves = get_number_of_leave_days(self.employee, self.leave_type,
  File "apps/erpnext/erpnext/hr/doctype/leave_application/leave_application.py", line 589, in get_number_of_leave_days
    elif half_day_date and half_day_date <= to_date:
TypeError: '<=' not supported between instances of 'str' and 'datetime.date'
  • One more bug was discovered for half-day while fixing:
    • Create a leave application (1st - 10th) spanning over 2 allocations (1st - 5th) (6th - 10th)
    • Choose Half Day for any date within the first allocation, say 1st
    • Total leave days come to 9.5. But while creating ledger entries this will add 0.5 to each ledger entry making it 10 because get_number_of_leave_days only checks if Half Day Date is less than To Date.
    • This adds 0.5 for both allocations. Fixed it to check if Half Day Date falls between From and To dates. So that it checks if half day actually falls in which allocation of the 2.

Missed this half-day edge case in #29439
Extended tests for separate ledger entry and leave ledger entry for intermediate expiry to check for half day.

…tion

- check if half day date exactly falls in between start and end dates to avoid considering it twice in separate ledger entries created when CF leaves have intermediate expiry or when application is created across 2 separate allocations

- cover half day case in tests
@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #30538 (9f3ef99) into develop (bfc34e1) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #30538      +/-   ##
===========================================
- Coverage    60.95%   60.81%   -0.15%     
===========================================
  Files         1083     1083              
  Lines        69126    69124       -2     
===========================================
- Hits         42137    42038      -99     
- Misses       26989    27086      +97     
Impacted Files Coverage Δ
.../hr/doctype/leave_application/leave_application.py 82.34% <100.00%> (+0.19%) ⬆️
...eport/item_variant_details/item_variant_details.py 31.32% <0.00%> (-53.02%) ⬇️
.../report/delayed_item_report/delayed_item_report.py 60.78% <0.00%> (-35.30%) ⬇️
...tch_item_expiry_status/batch_item_expiry_status.py 67.92% <0.00%> (-24.53%) ⬇️
...wise_balance_history/batch_wise_balance_history.py 67.79% <0.00%> (-23.73%) ⬇️
...payroll/doctype/income_tax_slab/income_tax_slab.py 83.33% <0.00%> (-16.67%) ⬇️
erpnext/accounts/doctype/bank/bank.py 71.42% <0.00%> (-14.29%) ⬇️
...saction/incorrect_balance_qty_after_transaction.py 88.37% <0.00%> (-11.63%) ⬇️
...xt/stock/report/stock_analytics/stock_analytics.py 80.19% <0.00%> (-10.90%) ⬇️
...pnext/accounts/report/gross_profit/gross_profit.py 80.24% <0.00%> (-4.94%) ⬇️
... and 25 more

@ruchamahabal ruchamahabal merged commit 6576327 into frappe:develop Apr 1, 2022
@ruchamahabal ruchamahabal deleted the half-day-leave-fix branch April 1, 2022 08:17
mergify bot pushed a commit that referenced this pull request Apr 1, 2022
…tion and fix half day edge case (#30538)

(cherry picked from commit 6576327)
ruchamahabal pushed a commit that referenced this pull request Apr 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant