feat: Formula-based Final Score calculation in Appraisals #1670
Conversation
This file contains 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
ruchamahabal
changed the title
ruchamahabal
requested changes
Apr 17, 2024
|
|
||
| if based_on_formula == 1: | ||
| final_score = eval(sanitized_formula, vars) | ||
| else: |
There was a problem hiding this comment.
Push this up before doing anything related to the formula. Non-formula eval is lighter so makes sense to check and execute that first.
| @@ -178,9 +179,35 @@ def calculate_avg_feedback_score(self, update=False): | |||
| self.db_update() | |||
|
|
|||
| def calculate_final_score(self): | |||
| final_score = (flt(self.total_score) + flt(self.avg_feedback_score) + flt(self.self_score)) / 3 | |||
| if self.appraisal_cycle: | |||
| apraisal_cycle_doc = frappe.get_doc("Appraisal Cycle", self.appraisal_cycle) | |||
There was a problem hiding this comment.
Suggested change
| apraisal_cycle_doc = frappe.get_doc("Appraisal Cycle", self.appraisal_cycle) | |
| appraisal_cycle_doc = frappe.get_doc("Appraisal Cycle", self.appraisal_cycle) |
fix typo everywhere
Comment on lines
189
to
203
| appraisal_cycle_fields = apraisal_cycle_doc.as_dict() | ||
| employee_fields = employee_doc.as_dict() | ||
|
|
||
| sanitized_formula = sanitize_expression(formula) | ||
|
|
||
| vars = { | ||
| "goal_score": flt(self.total_score), | ||
| "average_feedback_score": flt(self.avg_feedback_score), | ||
| "self_appraisal_score": flt(self.self_score), | ||
| } | ||
|
|
||
| vars.update(appraisal_cycle_fields) | ||
| vars.update(employee_fields) | ||
|
|
||
| vars = {key: flt(value) for (key, value) in vars.items()} |
There was a problem hiding this comment.
Suggested change
| appraisal_cycle_fields = apraisal_cycle_doc.as_dict() | |
| employee_fields = employee_doc.as_dict() | |
| sanitized_formula = sanitize_expression(formula) | |
| vars = { | |
| "goal_score": flt(self.total_score), | |
| "average_feedback_score": flt(self.avg_feedback_score), | |
| "self_appraisal_score": flt(self.self_score), | |
| } | |
| vars.update(appraisal_cycle_fields) | |
| vars.update(employee_fields) | |
| vars = {key: flt(value) for (key, value) in vars.items()} | |
| sanitized_formula = sanitize_expression(formula) | |
| data = frappe._dict({ | |
| "goal_score": flt(self.total_score), | |
| "average_feedback_score": flt(self.avg_feedback_score), | |
| "self_appraisal_score": flt(self.self_score), | |
| }) | |
| data.update(appraisal_cycle_doc.as_dict()) | |
| data.update(self.as_dict()) | |
| data.update(employee_doc.as_dict()) |
- updating as_dict() directly here saving some extra variables
- lets call vars -> data?
- Appraisal fields should also be available for eval -> added self.as_dict()
- Do we really need the flt() casting on line number 203. Not all fields in vars are float right?
There was a problem hiding this comment.
added flt because, adding some field that wasnt a float in expression was throwing incompatible data types error. e.g. employee.employee_name
vinyselopal
force-pushed
the
appraisal-enhancement
branch
from
f5173ec
to
862ebb1
Compare
vinyselopal
force-pushed
the
appraisal-enhancement
branch
from
862ebb1
to
5c8196f
Compare
vinyselopal
force-pushed
the
appraisal-enhancement
branch
2 times, most recently
from
9516507
to
51a688b
Compare
ruchamahabal
requested changes
Apr 18, 2024
Comment on lines
185
to
188
| appraisal_cycle_doc = frappe.get_doc("Appraisal Cycle", self.appraisal_cycle) | ||
| employee_doc = frappe.get_doc("Employee", self.employee) | ||
|
|
||
| formula = appraisal_cycle_doc.final_score_formula | ||
| based_on_formula = appraisal_cycle_doc.calculate_final_score_based_on_formula |
There was a problem hiding this comment.
Move this inside the else block. We don't need this at all if its not based on formula. For the formula checkbox you can do a db call directly instead of get_doc
There was a problem hiding this comment.
moved the statements not required to calculate the condition inside else block
vinyselopal
force-pushed
the
appraisal-enhancement
branch
2 times, most recently
from
012e060
to
cc42051
Compare
- remove unnecessary conditions - get cached doc for formula eval - rearrange related code blocks together
ruchamahabal
force-pushed
the
appraisal-enhancement
branch
from
6dbdc61
to
a2c2a8c
Compare
ruchamahabal
approved these changes
May 29, 2024
ruchamahabal
added a commit
that referenced
this pull request
May 29, 2024
…1670 feat: Formula-based Final Score calculation in Appraisals (backport #1670)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description: Added field in Appraisal Cycle to calculate final score for an employee appraisal using custom formula. Checking the field makes "Final Score Formula" field visible wherein you can add a Python Expression using autocompletion suggestions for various Employee master, Appraisal and Appraisal Cycle fields. This will automatically calculate the final score when creating new Appraisal
Closes #690
docs: https://frappehr.com/docs/v14/en/appraisal-cycle