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: validate department based on company in expense claim #2582

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

rs-rethik
Copy link

Issue:
allowing to submit expense claim with different departments with different companies
ref: 28450

Solution:
added filter for department field and validation for department based on company

Before:

before.webm

After:

after.webm

Backport needed for v15

@rs-rethik rs-rethik marked this pull request as ready for review January 6, 2025 06:00
@venkat102 venkat102 force-pushed the validate_department branch from d224f77 to 6880dfc Compare January 7, 2025 10:55
@@ -83,6 +88,14 @@ def set_status(self, update=False):
else:
self.status = status

def validate_company_and_department(self):
if self.department:
if self.company != frappe.db.get_value("Department", self.department, "company"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nesting not needed here? Can combine with line number 92 using "and"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nesting not needed here? Can combine with line number 92 using "and"

yes

@ruchamahabal
Copy link
Member

Can you fix tests related to this validation too?

@rs-rethik
Copy link
Author

Can you fix tests related to this validation too?

yes working on it

@venkat102 venkat102 force-pushed the validate_department branch from 5d3e23b to a4e8952 Compare January 10, 2025 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants