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

BUGFIX: Prevent multiple imports of the same remote asset in the frontend #5117

Merged
merged 5 commits into from
Jun 5, 2024
Merged

BUGFIX: Prevent multiple imports of the same remote asset in the frontend #5117

merged 5 commits into from
Jun 5, 2024

Conversation

so-grimm
Copy link
Contributor

@so-grimm so-grimm commented Jun 3, 2024 •
edited
Loading

This is a frontend fix for #5116 and prevents users from triggering multiple import processes for the same remote asset. It is not a sufficient fix to only prevent this in the frontend though, since it doesn't catch it, if two or more different users trigger the import for the same asset at the same time.

Changes:

  • select.js: add data attribute data-import-in-process to asset once import process has started and remove it when import is done
  • select.js: check for new data attribute and only start import process if attribute does not exist
  • select.js: add notification to inform user that asset is being imported
  • select.js: add notification as warning for user if import is already in process
  • Main.xlf: add new notification messages for english
  • Default.html: add id for notification container to be able to send notifications to it via js
  • Configuration.js: update hasConfiguration after configuration object was created, because otherwise it will always be false and the translations don't work

related: #5116

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

Info for testing:
You need to bundle the Neos.Neos assets to get the text for the notification messages.

  • navigate to the Neos.Neos package
  • run yarn
  • run yarn build

@so-grimm
Bugfix/5116: frontend fix for multiple imports of the same remote asset
69ee4d9 
- `select.js`: add data attribute `data-import-in-process` to asset once import process has started and remove it when import is done
- `select.js`: check for new data attribute and only start import process if attribute does not exist
- `select.js`: add notification to inform user that asset is being imported
- `select.js`: add notification as warning for user if import is already in process
- `Main.xlf`: add new notification messages for english
- `Default.html`: add id for notification container to be able to send notifications to it via js
- `Configuration.js`: update `hasConfiguration` after configuration object was created, because otherwise it will always be false and the translations don't work

Issue: neos#5116
@github-actions github-actions bot added the 8.3 label Jun 3, 2024
@so-grimm so-grimm changed the title Bugfix/5116: frontend fix for multiple imports of the same remote asset BUGFIX: frontend fix for multiple imports of the same remote asset Jun 3, 2024
@github-actions github-actions bot added the Bug label Jun 3, 2024
ahaeslich
ahaeslich approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@ahaeslich ahaeslich left a comment

Choose a reason for hiding this comment

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

+1 by reading

@ahaeslich ahaeslich changed the title BUGFIX: frontend fix for multiple imports of the same remote asset BUGFIX: Prevent multiple imports of the same remote asset in the frontend Jun 3, 2024
Sebobo
Sebobo requested changes Jun 3, 2024
View reviewed changes
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

Thx! Also for fixing the configuration issue.

We should just, when we anyway adjust the code not use more jQuery, but replace where its easy.

Neos.Media.Browser/Resources/Public/JavaScript/select.js Outdated Show resolved Hide resolved
@so-grimm so-grimm requested review from Sebobo and ahaeslich June 4, 2024 09:17
@crydotsnake crydotsnake requested review from crydotsnake and removed request for crydotsnake June 5, 2024 07:50
@so-grimm so-grimm force-pushed the bugfix/prevent-multiple-imports-of-same-asset branch from 0c2bee9 to 649ae4e Compare June 5, 2024 08:38
dlubitz
dlubitz approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

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

Thank you for this. Looks good to me by reading.

@ahaeslich ahaeslich enabled auto-merge June 5, 2024 10:13
Sebobo
Sebobo approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

⭐️⭐️⭐️⭐️⭐️

@ahaeslich ahaeslich merged commit ad66c9e into neos:8.3 Jun 5, 2024
9 checks passed
@Sebobo Sebobo deleted the bugfix/prevent-multiple-imports-of-same-asset branch June 5, 2024 10:14
@ahaeslich ahaeslich linked an issue Jun 5, 2024 that may be closed by this pull request
BUG: Prevent multiple/parallel imports of external asset source assets #5116
Closed
1 task
@crydotsnake crydotsnake mentioned this pull request Jun 13, 2024
Merged
6 tasks
@so-grimm
Copy link
Contributor Author

I noticed something strange today relating to this bugfix. In this PR I made changes to the Neos.Media.Browser package, but also to the Neos.Neos package. I assumed that assets would be bundled automatically via pipeline, so I didn't bundle them. Now I noticed that the Neos.Media.Browser changes are working just fine (because the are included directly and don't need bundling), but the Neos.Neos changes are not working, because they need to be bundled and that didn't happen. Without it, the notification messages don't show any text.
Can someone tell me how the workflow is supposed to work? Should I have bundled the assets myself and pushed them? And what do we do about it now? Would be great if someone could help me with that.

@dlubitz
Copy link
Contributor

dlubitz commented Jan 9, 2025

Very likely this change caused a bug in the selection of images out of the list view:
#5430

@dlubitz dlubitz mentioned this pull request Jan 9, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sebobo Sebobo Sebobo approved these changes

@ahaeslich ahaeslich ahaeslich approved these changes

@dlubitz dlubitz dlubitz approved these changes

@markusguenther markusguenther Awaiting requested review from markusguenther

@mhsdesign mhsdesign Awaiting requested review from mhsdesign

Assignees
No one assigned
Labels
8.3 Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Prevent multiple/parallel imports of external asset source assets
4 participants
@so-grimm @dlubitz @Sebobo @ahaeslich