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: broken patches #29067

Merged
merged 6 commits into from
Jan 25, 2022
Merged

fix: broken patches #29067

merged 6 commits into from
Jan 25, 2022

Conversation

saurabh6790
Copy link
Member

Please provide enough information so that others can review your pull request:

Explain the details for making this change. What existing problem does the pull request solve?

Screenshots/GIFs

@saurabh6790
Copy link
Member Author

@ankush fixed suggested changes !

@deepeshgarg007
Copy link
Member

@saurabh6790 can you fix the linting issues. You'll have to set up pre-commit locally as well. For this you can follow
https://github.com/frappe/erpnext/wiki/Pull-Request-Checklist#linting

@frappe frappe deleted a comment from mergify bot Jan 21, 2022
@ankush ankush self-assigned this Jan 21, 2022
with previous query rerunning would've caused all values to become 0.
@ankush
Copy link
Member

ankush commented Jan 21, 2022

@Mergifyio copy develop

@mergify
Copy link
Contributor

mergify bot commented Jan 21, 2022

copy develop

✅ Pull request copies have been created

@saurabh6790
Copy link
Member Author

@ankush are you working on this? It's been 27 days now!

+1 for adding the check of field type.

But I still believe, there is no need of two queries. The field type check and sql case statement will do job for you in one go.

@ankush ankush changed the title chore: patch fixes fix: broken patches Jan 25, 2022
@ankush ankush added the Skip Manual Testing The changes in this PR does not require manual testing label Jan 25, 2022
@ankush
Copy link
Member

ankush commented Jan 25, 2022

there is no need of two queries.

Added it before adding type check cause re-running could cause the entire column to become 0; best to leave it... it doesn't take much time and it runs one time only.

@ankush ankush merged commit 034773a into frappe:version-13-hotfix Jan 25, 2022
ankush added a commit that referenced this pull request Jan 25, 2022
* chore: patch fixes

(cherry picked from commit 8b5b146)

# Conflicts:
#	erpnext/patches/v13_0/make_homepage_products_website_items.py

* fix: remove desktop icons while deleting sales reports

(cherry picked from commit 5f72026)

* refactor: dont ignore dangerous exceptions in patches

(cherry picked from commit 0aa1ea8)

* fix: make patch kinda idempotent

with previous query rerunning would've caused all values to become 0.

* chore: conflicts

* fix: check type before patching

Co-authored-by: Saurabh <saurabh6790@gmail.com>
Co-authored-by: Ankush Menat <ankush@frappe.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Manual Testing The changes in this PR does not require manual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants