Skip to content

Conversation

walidsahli
Copy link
Contributor

Problem:
Tests introduced in commit [b7a63f7d60494c65889ba6ce3fa18847a0abbb62] (b7a63f7) are failing on runbot due to improper element selection and handling of image loading when the component is destroyed.

Solution:

  • Ensure the test properly targets the correct elements.
  • Handle cases where the image is still loading while the component is destroyed, preventing potential tracebacks.

Steps to Reproduce:

  1. Run the test suite on runbot.
  2. Observe test failures related to incorrect element selection or missing image references due to component destruction.

opw-4607020


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Mar 25, 2025

Pull request status dashboard

@walidsahli walidsahli force-pushed the 18.0-opw-4607020-fix_image_cropper_editor-wasa branch from 87125b4 to 1be9ebd Compare March 25, 2025 08:15
Comment on lines 147 to 149
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// in case the cropper is closed and the image just got loaded
// we return because the component is destroyed and `this.imageRef.el`
// is `null`
// Abort if the component has been destroyed in the meantime
// since `this.imageRef.el` is `null` when it is not mounted.

@walidsahli walidsahli force-pushed the 18.0-opw-4607020-fix_image_cropper_editor-wasa branch from 1be9ebd to 562b6b6 Compare March 25, 2025 08:32
@C3POdoo C3POdoo added the OE the report is linked to a support ticket (opw-...) label Mar 25, 2025
@dmo-odoo dmo-odoo marked this pull request as ready for review March 25, 2025 09:06
@C3POdoo C3POdoo requested a review from a team March 25, 2025 09:08
@walidsahli walidsahli force-pushed the 18.0-opw-4607020-fix_image_cropper_editor-wasa branch 23 times, most recently from f2d9f58 to fb2a59c Compare March 28, 2025 09:24
@walidsahli walidsahli force-pushed the 18.0-opw-4607020-fix_image_cropper_editor-wasa branch 10 times, most recently from d301cb0 to dc0a03a Compare March 28, 2025 10:58
Comment on lines 259 to 260
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change needed? It has a functional effect that the scroll is not sooth anymore.

Copy link
Contributor Author

@walidsahli walidsahli Mar 28, 2025

Choose a reason for hiding this comment

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

This is another issue i found while testing manually, the function was async but it will never resolve in case scroll is needed because scrollTo is not a promise.
removing the async will scroll but but due to smooth the image is loaded but still scrolling in progress.
One way to fix is to wait until scroll is finished (didn't check how), the other way is to simply use instant.
I took this decision because it was easier IMHO. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean it will never resolve? await undefined will resolve to undefined after one microstask tick. However, if scrollTo is not a promise, then it won't wait for it to be finished before resolving. Maybe that's what you meant? That it resolves and continues even though the scrolling isn't finished yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when i was testing it was not resolved at all, the execution stopped in the await this.scrollToInvisibleImage(); but i'm not able to reproduce as i deleted the images i was using to test earlier.
I reverted that part, this should be enough to fix the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked and scrollTo does return a Promise so if it is possible that scrollTo does not resolve then scrollToInvisibleImage won't resolve either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what happened. but i was randomly uploading images, now i can't reproduce anymore.

@walidsahli walidsahli force-pushed the 18.0-opw-4607020-fix_image_cropper_editor-wasa branch 2 times, most recently from 33037db to 63798a2 Compare March 28, 2025 12:59
**Problem**:
Tests introduced in commit [`b7a63f7d60494c65889ba6ce3fa18847a0abbb62`]
(odoo@b7a63f7)
are failing on **runbot** due to improper element selection and handling
of image loading when the component is destroyed.

**Solution**:
- Ensure the test properly targets the correct elements.
- Handle cases where the image is still loading while the component
  is destroyed, preventing potential tracebacks.

**Steps to Reproduce**:
1. Run the test suite on runbot.
2. Observe test failures related to incorrect element selection or
   missing image references due to component destruction.

**opw-4607020**
@walidsahli walidsahli force-pushed the 18.0-opw-4607020-fix_image_cropper_editor-wasa branch from 63798a2 to 587f20b Compare March 28, 2025 12:59
@dmo-odoo
Copy link
Contributor

@robodoo r+

@walidsahli
Copy link
Contributor Author

@robodoo up to 18.0

@robodoo
Copy link
Contributor

robodoo commented Mar 28, 2025

Forward-port disabled (via limit).

@walidsahli
Copy link
Contributor Author

these changes are applied in this FW from 18.1 up to master in this PR

@robodoo robodoo closed this in 4639637 Mar 28, 2025
gamarino pushed a commit to numaes/numa-public-odoo that referenced this pull request Mar 31, 2025
**Problem**:
Tests introduced in commit [`b7a63f7d60494c65889ba6ce3fa18847a0abbb62`]
(odoo/odoo@b7a63f7)
are failing on **runbot** due to improper element selection and handling
of image loading when the component is destroyed.

**Solution**:
- Ensure the test properly targets the correct elements.
- Handle cases where the image is still loading while the component
  is destroyed, preventing potential tracebacks.

**Steps to Reproduce**:
1. Run the test suite on runbot.
2. Observe test failures related to incorrect element selection or
   missing image references due to component destruction.

**opw-4607020**

closes odoo/odoo#203218

Signed-off-by: David Monjoie (dmo) <dmo@odoo.com>
@fw-bot fw-bot deleted the 18.0-opw-4607020-fix_image_cropper_editor-wasa branch April 4, 2025 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OE the report is linked to a support ticket (opw-...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants