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

Unable to publish/preview new assets when quick publish from tools config page editor #7

Conversation

srikanthgurram-17
Copy link
Contributor

@srikanthgurram-17 srikanthgurram-17 commented Nov 28, 2024

This is to solve the issue wcm-io/io.wcm.caconfig.editor#47.

  • Right now the ConfigurationReferenceProvider is called when we try to publish the page using tools --> config page
  • The ConfigurationReferenceProvider class is responsible to identify all the ca config references in a page
  • In future we need to support all the assets type referenced in ca config as well as ca config type reference we need to provide asset references which are outdated or newly added.
  • I have updated this class so that it finds both types.

@srikanthgurram-17
Copy link
Contributor Author

@stefanseifert Can you review this MR please?

@srikanthgurram-17
Copy link
Contributor Author

@stefanseifert gentle reminder, As I am away after this Friday until next year to do any changes.

Copy link
Member

@stefanseifert stefanseifert left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

there are some sonar findings (which unfortunately are not visible for external PRs directly in the PR comments), please have a look at https://sonarcloud.io/summary/new_code?id=wcm-io_io.wcm.caconfig.extensions&branch=feature%2Fassetreferences-publishing-support

overall this is the right approach, but there are some open issues.

@stefanseifert stefanseifert self-assigned this Dec 5, 2024
@stefanseifert
Copy link
Member

i've refactored the code to cover more edge cases:

  • detect asset references in configuration collections and nested configurations (stored as nested resources)
  • detect asset references not only in string properties, but also in string array properties
  • de-duplicate list of collected assets in case the same asset is references multiple times

i also moved the actual reference detection logic to a separate class to not clutter the main class too much, and make the edge cases testable independent of the other (complex) parts of the reference provider.

additionally, i added a new OSGi configuration option which allows to enable the detection of asset, but is disabled by default. so, we can test it a bit if it has performance impact, before enable it for everyone maybe in a later release.

@stefanseifert stefanseifert merged commit f0dea9a into wcm-io:develop Dec 9, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants