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

BUGFIX: Update of usage of previous asset when an asset property is updated #11

Merged
merged 2 commits into from
Apr 5, 2023

Conversation

andrehoffmann30
Copy link
Contributor

@andrehoffmann30 andrehoffmann30 commented Mar 8, 2023

Currently if you replace or remove an image from a node the asset usage of the first used image is not updated, when the changes are published. For example if an image is added to a node the usage is correctly set to 1. When this image is then replaced by an other image, the asset usage of the first image stays as 1 and the second image gets a correct asset usage of 1. The expected behaviour is, that the asset usage of the first image should go from 1 to 0.

In this pr the correct class for the nodeDiscarded Signal (see here https://github.com/Flowpack/Flowpack.Neos.AssetUsage/pull/11/files#diff-69e3e8da81e0c1cc4f7aff2f36a95d4d52edd92ea5628b312a09970b77cf5868R35) is added and the code order in the beforeNodePublishing function is adjusted. The the old image usage needs to be update (see here https://github.com/Flowpack/Flowpack.Neos.AssetUsage/pull/11/files#diff-ad82dc394f028c9fdc2b8d83ceb45b47980d9b94954827a3126feab8b1afb102L216) before one can check whether a new image is added (see the checks here https://github.com/Flowpack/Flowpack.Neos.AssetUsage/pull/11/files#diff-ad82dc394f028c9fdc2b8d83ceb45b47980d9b94954827a3126feab8b1afb102R216).
With these changes the asset usage for the old image is updated correctly, when a replacement or removal of this image is published. I think, this pr should also fix the issue #2 .

@andrehoffmann30 andrehoffmann30 marked this pull request as ready for review March 9, 2023 08:08
@Sebobo
Copy link
Member

Sebobo commented Mar 21, 2023

Can you adjust the title and description of the PR with more details?
Is it now a task or bugfix?
And what reproducible error does it fix?

@andrehoffmann30 andrehoffmann30 changed the title TASK: adjust update behaviour of asset usage BUGFIX: fix update of asset usage when publishing a node with an image property Apr 2, 2023
@andrehoffmann30
Copy link
Contributor Author

Hallo @Sebobo I updated the title and description of my pr. I hope this is now more understandable. Sorry that it took me 2 weeks the react. Please contact me if you have any questions. Thank you.

@crydotsnake crydotsnake added the bug Something isn't working label Apr 2, 2023
@daniellienert
Copy link
Contributor

@Sebobo Hey seb, do you have any further objections? Its already tested in our customer project.

@Sebobo Sebobo changed the title BUGFIX: fix update of asset usage when publishing a node with an image property BUGFIX: Update of usage of previous asset when an asset property is updated Apr 5, 2023
@Sebobo
Copy link
Member

Sebobo commented Apr 5, 2023

OK, looks good. I couldn't reproduce the issue with replacing an asset but the discarding part and the order change makes sense. Thx!

@Sebobo Sebobo merged commit 3f7b733 into Flowpack:main Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants