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: use max function to get default company address #34116

Merged
merged 3 commits into from
Mar 14, 2023
Merged

fix: use max function to get default company address #34116

merged 3 commits into from
Mar 14, 2023

Conversation

prateekkaramchandani
Copy link
Contributor

Non-Primary company address is being auto fetched in sales invoice instead of primary company address. This Pull Request fixes that.

Before:

before.mp4

After:

after.mp4

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Feb 17, 2023
@codecov
Copy link

codecov bot commented Feb 18, 2023 •
edited
Loading

Codecov Report

Merging #34116 (b93c18b) into develop (2942cee) will decrease coverage by 0.20%.
The diff coverage is 0.00%.

❗ Current head b93c18b differs from pull request most recent head e004297. Consider uploading reports for the commit e004297 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #34116      +/-   ##
===========================================
- Coverage    64.09%   63.89%   -0.20%     
===========================================
  Files          810      817       +7     
  Lines        59372    59337      -35     
===========================================
- Hits         38052    37912     -140     
- Misses       21320    21425     +105
Impacted Files Coverage Δ
erpnext/setup/doctype/company/company.py 73.50% <0.00%> (ø)

... and 41 files with indirect coverage changes

barredterra
barredterra reviewed Mar 3, 2023
View reviewed changes
Copy link
Collaborator

@barredterra barredterra left a comment

Choose a reason for hiding this comment

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

Makes sense 👍🏼

Could you add a test case in test_company.py to make sure this doesn't happen again?

@deepeshgarg007
Copy link
Member

@prateekkaramchandani This looks like an essential fix, can you please add the tests?

@ankush
Copy link
Member

ankush commented Mar 14, 2023

Looks like I broke this 😔 because cmp had x and y flipped and I got confused.

897e90a

@ankush ankush removed the needs-tests This PR needs automated unit-tests. label Mar 14, 2023
ankush
ankush previously approved these changes Mar 14, 2023
View reviewed changes
@ankush ankush added the backport version-14-hotfix backport to version 14 label Mar 14, 2023
@ankush ankush merged commit f1752fc into frappe:develop Mar 14, 2023
@mergify mergify bot mentioned this pull request Mar 14, 2023
Merged
ankush added a commit that referenced this pull request Mar 15, 2023
@mergify @prateekkaramchandani @ankush
fix: use max function to get default company address (backport #34116) (
ba2fd71 
#34452)

* fix: use max function to get default company address

(cherry picked from commit b93c18b)

* test: add test for primary address sorting

(cherry picked from commit e004297)

---------

Co-authored-by: Prateek <40106895+prateekkaramchandani@users.noreply.github.com>
Co-authored-by: Ankush Menat <ankush@frappe.io>
frappe-pr-bot pushed a commit that referenced this pull request Mar 21, 2023
@frappe-bot
chore(release): Bumped to Version 14.19.0
e271935 
# [14.19.0](v14.18.3...v14.19.0) (2023-03-21)

### Bug Fixes

* **client:** Amount calculation for 0 qty debit notes ([#34455](#34455)) ([d24f4d2](d24f4d2))
* difference amount calculation for company currency accounts ([9ab7bff](9ab7bff))
* don't map item row having `0` qty ([7611a49](7611a49))
* E-commerce issue with Item Variants ([53c3fff](53c3fff))
* german translations ([#34312](#34312)) ([dd0c833](dd0c833))
* hide `+` button based on `Blanket Order Type` ([daa1bb8](daa1bb8))
* incorrect depr schedules after asset repair [v14] ([#34527](#34527)) ([560df63](560df63)), closes [#30838](#30838)
* Multiple accounting dimension filtering in AR/AP reports ([#34464](#34464)) ([f146479](f146479))
* Overallocation of 'qty' from Cr Notes to Parent Invoice ([848e56b](848e56b))
* patch depends on Currency Exchange Settings ([#34494](#34494)) ([4acde44](4acde44))
* POS not picking up pos profile company address instead fetch any random company address ([#34521](#34521)) ([01f4cc7](01f4cc7))
* Update account number from parent company ([#34474](#34474)) ([55d002c](55d002c))
* use max function to get default company address (backport [#34116](#34116)) ([#34452](#34452)) ([ba2fd71](ba2fd71))

### Features

* add field `Over Order Allowance (%)` in `Buying Settings` ([da915f1](da915f1))
* add field `Over Order Allowance (%)` in `Selling Settings` ([46b5ba9](46b5ba9))
* bank reconciliation and plaid changes ([#33986](#33986)) ([9b608ea](9b608ea))
* consider `over_order_allowance` while validating order qty ([932639b](932639b))
* consider `over_order_allowance` while validating sales order qty ([09b577a](09b577a))
* Support for Alternative Items in Quotation ([#33874](#33874)) ([9f7da21](9f7da21))

### Performance Improvements

* index against_sales_invoice field on DN items (backport [#34509](#34509)) ([#34510](#34510)) ([baa789b](baa789b))
babureddyks pushed a commit to babureddyks/Check_Engine that referenced this pull request Mar 28, 2023
@frappe-bot
chore(release): Bumped to Version 14.19.0
475425c 
# [14.19.0](frappe/erpnext@v14.18.3...v14.19.0) (2023-03-21)

### Bug Fixes

* **client:** Amount calculation for 0 qty debit notes ([#34455](frappe/erpnext#34455)) ([d24f4d2](frappe/erpnext@d24f4d2))
* difference amount calculation for company currency accounts ([9ab7bff](frappe/erpnext@9ab7bff))
* don't map item row having `0` qty ([7611a49](frappe/erpnext@7611a49))
* E-commerce issue with Item Variants ([53c3fff](frappe/erpnext@53c3fff))
* german translations ([#34312](frappe/erpnext#34312)) ([dd0c833](frappe/erpnext@dd0c833))
* hide `+` button based on `Blanket Order Type` ([daa1bb8](frappe/erpnext@daa1bb8))
* incorrect depr schedules after asset repair [v14] ([#34527](frappe/erpnext#34527)) ([560df63](frappe/erpnext@560df63)), closes [#30838](frappe/erpnext#30838)
* Multiple accounting dimension filtering in AR/AP reports ([#34464](frappe/erpnext#34464)) ([f146479](frappe/erpnext@f146479))
* Overallocation of 'qty' from Cr Notes to Parent Invoice ([848e56b](frappe/erpnext@848e56b))
* patch depends on Currency Exchange Settings ([#34494](frappe/erpnext#34494)) ([4acde44](frappe/erpnext@4acde44))
* POS not picking up pos profile company address instead fetch any random company address ([#34521](frappe/erpnext#34521)) ([01f4cc7](frappe/erpnext@01f4cc7))
* Update account number from parent company ([#34474](frappe/erpnext#34474)) ([55d002c](frappe/erpnext@55d002c))
* use max function to get default company address (backport [#34116](frappe/erpnext#34116)) ([#34452](frappe/erpnext#34452)) ([ba2fd71](frappe/erpnext@ba2fd71))

### Features

* add field `Over Order Allowance (%)` in `Buying Settings` ([da915f1](frappe/erpnext@da915f1))
* add field `Over Order Allowance (%)` in `Selling Settings` ([46b5ba9](frappe/erpnext@46b5ba9))
* bank reconciliation and plaid changes ([#33986](frappe/erpnext#33986)) ([9b608ea](frappe/erpnext@9b608ea))
* consider `over_order_allowance` while validating order qty ([932639b](frappe/erpnext@932639b))
* consider `over_order_allowance` while validating sales order qty ([09b577a](frappe/erpnext@09b577a))
* Support for Alternative Items in Quotation ([#33874](frappe/erpnext#33874)) ([9f7da21](frappe/erpnext@9f7da21))

### Performance Improvements

* index against_sales_invoice field on DN items (backport [#34509](frappe/erpnext#34509)) ([#34510](frappe/erpnext#34510)) ([baa789b](frappe/erpnext@baa789b))
@ankush ankush mentioned this pull request Oct 20, 2023
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@barredterra barredterra barredterra left review comments

@ankush ankush ankush approved these changes

Assignees
No one assigned
Labels
backport version-14-hotfix backport to version 14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@prateekkaramchandani @deepeshgarg007 @ankush @barredterra