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

perf: dont reload doc when already saving #21869

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

sagarvora
Copy link
Contributor

@sagarvora sagarvora commented Jul 30, 2023

Issue

Document gets reloaded in the following code even though it's already saving:

if (data.modified !== cur_frm.doc.modified) {
if (!cur_frm.is_dirty()) {
cur_frm.reload_doc();

Underlying problem is in following sequence:

  • first the realtime update is received on commit
  • then the save response is received

The problematic code was introduced in #20126 to ensure that workflows work as expected. But frappe.ui.form.is_saving should be false during that so it should still work as expected.

Additionally, the above code causes refresh event to get triggered twice:

  • once after save
  • once inside reload_doc

That leads to issues like this:

image

Solution

Don't trigger reload if form is already saving.

@sagarvora sagarvora requested review from surajshetty3416 and a team July 30, 2023 13:03
@sagarvora sagarvora added the backport version-14-hotfix backport to version 14 label Jul 30, 2023
@sagarvora sagarvora requested review from ankush and removed request for a team July 30, 2023 13:03
@codecov
Copy link

codecov bot commented Jul 30, 2023

Codecov Report

Merging #21869 (f8b4cd1) into develop (8418858) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #21869   +/-   ##
========================================
  Coverage    61.82%   61.83%           
========================================
  Files          765      765           
  Lines        72975    72974    -1     
  Branches      6291     6290    -1     
========================================
+ Hits         45120    45123    +3     
+ Misses       24278    24275    -3     
+ Partials      3577     3576    -1     
Flag Coverage Δ
server-ui 28.12% <ø> (ø)
ui-tests 51.71% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ankush ankush merged commit 97eefda into frappe:develop Jul 31, 2023
19 of 20 checks passed
@ankush ankush deleted the dont-reload-when-saving branch July 31, 2023 06:19
mergify bot pushed a commit that referenced this pull request Jul 31, 2023
mergify bot pushed a commit that referenced this pull request Jul 31, 2023
ankush pushed a commit that referenced this pull request Jul 31, 2023
(cherry picked from commit 97eefda)

Co-authored-by: Sagar Vora <sagar@resilient.tech>
ankush pushed a commit that referenced this pull request Jul 31, 2023
(cherry picked from commit 97eefda)

Co-authored-by: Sagar Vora <sagar@resilient.tech>
frappe-pr-bot pushed a commit that referenced this pull request Aug 1, 2023
# [14.43.0](v14.42.0...v14.43.0) (2023-08-01)

### Bug Fixes

* better fetch_if_empty (old fix [#19586](#19586)) ([0930521](0930521))
* call cache() function ([74a26af](74a26af))
* **console:** dont commit when exception is raised and unconditionally check query ([#21850](#21850)) ([#21851](#21851)) ([e6388e7](e6388e7))
* **doctype:** Allow cached_property decorator in controllers ([#21881](#21881)) ([#21883](#21883)) ([fdc3c6c](fdc3c6c))
* fallback to mariadb if not defined ([a9526d6](a9526d6))
* include languages when clearing website cache ([31fb147](31fb147))
* invalidate cache when language is updated ([30d9b08](30d9b08))
* Parameter {function} get translated and format doesn't work ([#21859](#21859)) ([#21867](#21867)) ([b98a330](b98a330))
* removed the output field from the console log ([#21600](#21600)) ([#21855](#21855)) ([7d669c9](7d669c9))
* show full name instead of first/last name (backport [#21828](#21828)) ([#21831](#21831)) ([5db0bd0](5db0bd0))
* **web_form:** Allow Jinja extension of header buttons ([fb10b61](fb10b61))
* welcome workspace & misc fixes backport ([0048037](0048037))

### Features

* added specific kanban board as quick link in workspace ([#21721](#21721)) ([986b602](986b602))
* **sec:** log IP addresses of login/logout activities (backport [#21844](#21844)) ([#21845](#21845)) ([b34779f](b34779f))
* setting for force web capture mode for camera uploads (backport [#21378](#21378)) ([#21875](#21875)) ([a32915b](a32915b))

### Performance Improvements

* dont reload doc when already saving ([#21869](#21869)) ([#21877](#21877)) ([537bf07](537bf07))
* drop `ifnull` from `IS SET` filter ([#21822](#21822)) ([#21835](#21835)) ([42ab839](42ab839))
* skip reset_seen for new doc ([#21832](#21832)) ([#21833](#21833)) ([2ab2071](2ab2071))
frappe-pr-bot pushed a commit that referenced this pull request Aug 1, 2023
## [13.58.4](v13.58.3...v13.58.4) (2023-08-01)

### Bug Fixes

* fallback to unscrubbed table fieldname if label not found ([#21826](#21826)) ([a8091bb](a8091bb))
* removed the output field from the console log ([#21600](#21600)) ([#21854](#21854)) ([1995035](1995035))

### Performance Improvements

* dont reload doc when already saving ([#21869](#21869)) ([#21876](#21876)) ([706892d](706892d))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants