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.

Sign up for GitHub

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

Jump to bottom

fix(Timesheet): warn user if billing hours > actual hours instead of resetting #38239

Merged
merged 2 commits into from
Nov 21, 2023
Merged

fix(Timesheet): warn user if billing hours > actual hours instead of resetting #38239

merged 2 commits into from
Nov 21, 2023

Conversation

ruchamahabal
Copy link
Member

@ruchamahabal ruchamahabal commented Nov 21, 2023 •
edited
Loading

reverting changes in #38134 and replacing it with an alert to warn users if billing hours > actual hours to avoid oversight

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Nov 21, 2023
@ruchamahabal ruchamahabal removed the needs-tests This PR needs automated unit-tests. label Nov 21, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #38239 (025eb90) into develop (56e991b) will increase coverage by 0.00%.
Report is 2 commits behind head on develop.
The diff coverage is 91.66%.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop   #38239   +/-   ##
========================================
  Coverage    67.26%   67.26%           
========================================
  Files          757      757           
  Lines        60505    60507    +2     
========================================
+ Hits         40700    40702    +2     
  Misses       19805    19805
Files Coverage Δ
erpnext/assets/doctype/asset/asset.py 80.44% <100.00%> (ø)
erpnext/controllers/buying_controller.py 83.94% <ø> (ø)
erpnext/projects/doctype/timesheet/timesheet.py 78.34% <66.66%> (-0.23%) ⬇️

... and 1 file with indirect coverage changes

@ruchamahabal ruchamahabal merged commit ac91030 into frappe:develop Nov 21, 2023
14 checks passed
mergify bot pushed a commit that referenced this pull request Nov 21, 2023
@ruchamahabal @mergify
fix(Timesheet): warn user if billing hours > actual hours instead of …
6d06d03 
…resetting (#38239)

* revert: "fix(Timesheet): reset billing hours equal to hours if they exceed actual hours"

This reverts commit 0ec8034.

* fix: warn user if billing hours > actual hours

(cherry picked from commit ac91030)
mergify bot pushed a commit that referenced this pull request Nov 21, 2023
@ruchamahabal @mergify
fix(Timesheet): warn user if billing hours > actual hours instead of …
cdda5df 
…resetting (#38239)

* revert: "fix(Timesheet): reset billing hours equal to hours if they exceed actual hours"

This reverts commit 0ec8034.

* fix: warn user if billing hours > actual hours

(cherry picked from commit ac91030)
This was referenced Nov 21, 2023
Merged
Merged
mergify bot pushed a commit that referenced this pull request Nov 21, 2023
@ruchamahabal @mergify
fix(Timesheet): warn user if billing hours > actual hours instead of …
4d27018 
…resetting (#38239)

* revert: "fix(Timesheet): reset billing hours equal to hours if they exceed actual hours"

This reverts commit 0ec8034.

* fix: warn user if billing hours > actual hours

(cherry picked from commit ac91030)
@mergify mergify bot mentioned this pull request Nov 21, 2023
Merged
ruchamahabal added a commit that referenced this pull request Nov 21, 2023
@mergify @ruchamahabal
fix(Timesheet): warn user if billing hours > actual hours instead of …
e08f114 
…resetting (backport #38239) (#38240)

fix(Timesheet): warn user if billing hours > actual hours instead of resetting  (#38239)

* revert: "fix(Timesheet): reset billing hours equal to hours if they exceed actual hours"

This reverts commit 0ec8034.

* fix: warn user if billing hours > actual hours

(cherry picked from commit ac91030)

Co-authored-by: Rucha Mahabal <ruchamahabal2@gmail.com>
ruchamahabal added a commit that referenced this pull request Nov 21, 2023
@mergify @ruchamahabal
fix(Timesheet): warn user if billing hours > actual hours instead of …
1f2f5d8 
…resetting (backport #38239) (#38241)

fix(Timesheet): warn user if billing hours > actual hours instead of resetting  (#38239)

* revert: "fix(Timesheet): reset billing hours equal to hours if they exceed actual hours"

This reverts commit 0ec8034.

* fix: warn user if billing hours > actual hours

(cherry picked from commit ac91030)

Co-authored-by: Rucha Mahabal <ruchamahabal2@gmail.com>
ruchamahabal added a commit that referenced this pull request Nov 21, 2023
@mergify @ruchamahabal
fix(Timesheet): warn user if billing hours > actual hours instead of …
a9429e1 
…resetting (backport #38239) (#38242)

fix(Timesheet): warn user if billing hours > actual hours instead of resetting  (#38239)

* revert: "fix(Timesheet): reset billing hours equal to hours if they exceed actual hours"

This reverts commit 0ec8034.

* fix: warn user if billing hours > actual hours

(cherry picked from commit ac91030)

Co-authored-by: Rucha Mahabal <ruchamahabal2@gmail.com>
frappe-pr-bot pushed a commit that referenced this pull request Nov 21, 2023
@frappe-bot
chore(release): Bumped to Version 13.54.12
01fc5f6 
## [13.54.12](v13.54.11...v13.54.12) (2023-11-21)

### Bug Fixes

* `get_previous_allocation` return value ([6cb8a40](6cb8a40))
* backward compatible query without pluck ([958db77](958db77))
* pass check permission in render_address ([65ae8d9](65ae8d9))
* **Timesheet:** reset billing hours equal to hours if they exceed actual hours (backport [#38134](#38134)) ([#38154](#38154)) ([1e43605](1e43605))
* **Timesheet:** warn user if billing hours > actual hours instead of resetting  (backport [#38239](#38239)) ([#38242](#38242)) ([a9429e1](a9429e1))
* use `get_all` instead of `get_list` ([4b8ed0f](4b8ed0f))

### Performance Improvements

* index most queried fields in Leave Ledger Entry ([eea7bbc](eea7bbc))
* limit rows to 1 for cf leave expiry query ([635c3d5](635c3d5))
frappe-pr-bot pushed a commit that referenced this pull request Nov 21, 2023
@frappe-bot
chore(release): Bumped to Version 15.3.0
33869a2 
# [15.3.0](v15.2.0...v15.3.0) (2023-11-21)

### Bug Fixes

* attributes were mandatory for manufacturers ([00b9c23](00b9c23))
* issue occured when creating supplier with contact details ([aaccfeb](aaccfeb))
* overallocation on Payment with PO/SO ([337707b](337707b))
* pass check permission in render_address ([a420e13](a420e13))
* payment entry rounding error ([384d6b5](384d6b5))
* set asset's valuation_rate according to asset quantity (backport [#38254](#38254)) ([#38256](#38256)) ([c60aaa7](c60aaa7))
* set default asset quantity as 1 [dev] (backport [#38223](#38223)) ([#38226](#38226)) ([99bf63e](99bf63e))
* Suppier name was not taken when creating address from supplier ([2b94489](2b94489))
* Supplier Quotation fields ([#37963](#37963)) ([aef955c](aef955c))
* test case for rounded total with cash disc ([eab18e6](eab18e6))
* **Timesheet:** reset billing hours equal to hours if they exceed actual hours (backport [#38134](#38134)) ([#38153](#38153)) ([5b7b431](5b7b431))
* **Timesheet:** warn user if billing hours > actual hours instead of resetting  (backport [#38239](#38239)) ([#38241](#38241)) ([1f2f5d8](1f2f5d8))
* TypeError in Subcontracting Receipt (backport [#38200](#38200)) ([#38208](#38208)) ([3f57a7e](3f57a7e))
* update modified timestamp ([a492e57](a492e57))
* **ux:** `Task` creation from `Timesheet` (backport [#38207](#38207)) ([#38211](#38211)) ([e272041](e272041))
* valuation rate for FG item for subcontracting receipt (backport [#38244](#38244)) ([#38245](#38245)) ([ed7b845](ed7b845))
* valuation rate in report Item Prices ([#38161](#38161)) ([f71234e](f71234e))
* wrong round off and rounded total ([70eccf7](70eccf7))

### Features

* add `Supplier Delivery Note` field in SCR (backport [#38127](#38127)) ([#38156](#38156)) ([b89a4a7](b89a4a7))
* Add accounting dimensions to Supplier Quotation ([7d4ac7e](7d4ac7e))
@barredterra
Copy link
Collaborator

@ruchamahabal I think this would make more sense as a helper in the UI. If a user manually changes the hours, billing hours could automatically be adjusted accordingly. As a backend validation, this takes a lot of flexibility away. For example, in our workflow, where timesheets get created by the backend, users will now start seeing this message quite often.

frappe-pr-bot pushed a commit that referenced this pull request Nov 22, 2023
@frappe-bot
chore(release): Bumped to Version 14.49.0
7392f9c 
# [14.49.0](v14.48.1...v14.49.0) (2023-11-22)

### Bug Fixes

* add revaluation journal filter in Payable report ([d0698b3](d0698b3))
* attributes were mandatory for manufacturers ([430980a](430980a))
* duplicate field in `Closing Stock Balance` ([#38105](#38105)) ([1f16c47](1f16c47))
* incorrect incoming rate for serial and batch items in standalone debit note ([#38121](#38121)) ([9a34518](9a34518))
* pass check permission in render_address ([1ccd5e4](1ccd5e4))
* payment entry rounding error ([49735bc](49735bc))
* remove ESS role when not mapped to employee (backport [#37867](#37867)) ([#38132](#38132)) ([bc01007](bc01007))
* round `unreconciled_amount` before asserting ([392ee2e](392ee2e))
* set asset's valuation_rate according to asset quantity (backport [#38254](#38254)) ([#38255](#38255)) ([00def82](00def82))
* set default asset quantity as 1 [v14] ([#38224](#38224)) ([3daf6f8](3daf6f8))
* show party values when naming by is not naming series ([dd76695](dd76695))
* Supplier Quotation fields ([#37963](#37963)) ([883eaee](883eaee))
* test case for rounded total with cash disc ([9b5185a](9b5185a))
* **Timesheet:** reset billing hours equal to hours if they exceed actual hours (backport [#38134](#38134)) ([#38152](#38152)) ([c7c751e](c7c751e))
* **Timesheet:** warn user if billing hours > actual hours instead of resetting  (backport [#38239](#38239)) ([#38240](#38240)) ([e08f114](e08f114))
* update modified timestamp ([2849e0d](2849e0d))
* valuation rate in report Item Prices ([#38161](#38161)) ([a70696e](a70696e))
* wrong round off and rounded total ([296433a](296433a))

### Features

* add `Supplier Delivery Note` field in SCR (backport [#38127](#38127)) ([#38155](#38155)) ([8d4a19c](8d4a19c))
* Add accounting dimensions to Supplier Quotation ([51e33e1](51e33e1))
* virtual parent doctype ([8dbf2ce](8dbf2ce))
@ruchamahabal
Copy link
Member Author

If a user manually changes the hours, billing hours could automatically be adjusted accordingly.

So this issue originally sprouted when the user created his timesheet, saved --> the billing hours were set equal to hours on save
Then he went and reduced the hours in a row --> billing hours were not reset on the server-side since it already had value

User noticed total billing hours > total hours and thought it was a bug because one of the rows had Is Billable unchecked and thought it was still getting considered. But it was actually this issue where billing hours do not change if you reduce hours later.

So added this warning on the server-side. Won't stop you from adding billing hours > hours. Problem with auto-adjusting billing hours directly is when users want to report more billing hours than actual hours for customer and they rely on the UI. But this warning could perhaps be moved to client-side validate

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
backport version-13-hotfix backport version-14-hotfix backport to version 14 backport version-15-hotfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ruchamahabal @barredterra