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

update CMS schema #2543

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GCavailles
Copy link

Here is the update of the cms schema following our last release.

@GCavailles GCavailles requested a review from a team as a code owner December 2, 2024 16:05
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.78%. Comparing base (28ffcb9) to head (784afe4).
Report is 4 commits behind head on main.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

},
"ruleFunctionalContentActionEnabled": {
"type": "boolean",
"description": "Enables the custom rule action of type 'functional content update' (UPDATE_FUNCTIONAL_CONTENT). Should be enabled only if the application implements this custom action type",
Copy link
Contributor

@kpanot kpanot Dec 3, 2024

Choose a reason for hiding this comment

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

This cannot be in the base schema because is referring custom Rule that are not part of Otter package.
Do you think it would be possible to have an interface more generic like:

{
  "actionActivation": {
    "type": "object"
    "description": "List of Rules Engine actions to explicitly enable or disable"
    "patternProperties": {
      "^[A-Z_0-9]+$": { "type": "boolean" }
    }
  }
}

Which would lead, for this specific case, to a config like:

{
  "actionActivation": {
    "UPDATE_FUNCTIONAL_CONTENT": true
  }
}

Copy link

Choose a reason for hiding this comment

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

Hi,

this is already in prod so we will not change the structure.

But anyway the reference of a custom rule that is part or not part of the otter package is irrelevant here. This file describes the input provided by any blueprint to the CMS. It is not linked directly to otter but as all DES applications use otter, and 90% of them are meant to be imported in the CMS, it is in this repo for convenience.

If you take a look at the properties already in place it already refers to things that are not linked to otter.
For instance "functionalContentsFolder", functional contents are not handled by otter but still, we have this property.
"placeholderCssFiles" is completely linked to the way we preview experience fragments in the CMS ... not much to do with otter either.

The goal of this file is for the blueprint to configure the DES AEM plugin, therefore it makes sense that it contains properties that are very specific to us.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is already in prod so we will not change the structure.

If the PR containing the specification of an option is done after the releasing of this option in production, it sounds a bit like an issue in the process. Maybe doing the PR of the specification before releasing the option would help to handle the feedbacks and avoid mistake.
I would suggest to review the way it is done on that part.

But anyway the reference of a custom rule that is part or not part of the otter package is irrelevant here.

Yes it is relevant, in the same way you can not have a PR merged on Angular including naming/specificities of a single application, we can not accept an option/description pointing explicitly a rule implemented in one application.

"placeholderCssFiles" is completely linked to the way we preview experience fragments in the CMS ... not much to do with otter either.

I disagree on the fact that it is not linked to Otter. The communication with the CMS on feature provided by Otter (or on the way to bundle application for example) is in the scope of Otter.
This example is regarding the way display a placeholder that Otter will display in runtime.

If you take a look at the properties already in place it already refers to things that are not linked to otter.
For instance "functionalContentsFolder", functional contents are not handled by otter but still, we have this property.

Yes, this is not something, for my point of view, that should have been accepted in a file hosted on this repository, mainly because it is a concept of a single product.
This is an example of something we should not do so we can use it to continue to include things valid only for a single app or maybe to rethink the way the specification is managed.
I would personally go for the second option.

The goal of this file is for the blueprint to configure the DES AEM plugin, therefore it makes sense that it contains properties that are very specific to us.

Not sure to see why it would justify to have an option for a single product/app.
If tomorrow another product will come with another option specific I don't see how you will check/document that this options is not for others.
This is linked to the previous points, having options dedicated to the AEM Plugin makes sense, this is also the goal of Otter to facilitate the integration of applications within AEM Plugin and DGP Hosting; but this is valid only if the options fit (and can be used in) all Otter based application. Here it is not the case definitively.
I am not saying that you should not allow specificities, I say that we can make it in a much more clever way and document it properly on Otter side as well (as it was done for the pointed out "placeholderCssFiles" for example).

For this case specifically, as explained in my previous comment, it is linked to a feature of Otter that enables custom action handlers, we can have a plenty of custom handler in different application, so it is really dommage to have a dedicated option for a dedicated implementation for one product/application of a specific action type.

Copy link

Choose a reason for hiding this comment

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

I think there is really a misunderstanding on this file.

This file describes some input configuration for the AEM DES PLUGIN. It has nothing to do with otter, it has nothing to do with the ui framework used to develop the application, it is not provided or generated by otter. Therefore my team is the only owner of this file and its format.

You always take as argument that this is a flag specific to a single application, this is not the case, this is a flag specific to the AEM DES PLUGIN. We do not care how the application implements or not the result of this flag. This flag will influence the AEM DES PLUGIN UI and will have our plugin provide in output new action types in the ruleset.json. Then up to the application to handle it, using otter or not. This has nothing specific to refx and nothing specific to otter, we as the plugin team specify the output we generate when this flag is set. Any application can then implement the necessary to handle the output.

If the PR containing the specification of an option is done after the releasing of this option in production, it sounds a bit like an issue in the process. Maybe doing the PR of the specification before releasing the option would help to handle the feedbacks and avoid mistake. I would suggest to review the way it is done on that part.

Indeed we forgot to update this schema as part of our development process, our bad, mistakes happen. But this schema is not our specification, it is one output of our specification.

I disagree on the fact that it is not linked to Otter. The communication with the CMS on feature provided by Otter (or on the way to bundle application for example) is in the scope of Otter. This example is regarding the way display a placeholder that Otter will display in runtime.

If otter did not exist we would still have this property in this file. Also when otter renders the content in the placeholder it does not add the css files specified in this property. It's up to the application to add manually these files in its index.html. This property does not influence the runtime, it influences only the preview render of a content in the AEM DES PLUGIN. Up to the application then to include in its index.html the same files that are specified in this property.

Then I agree that regarding metadata files we develop them together with otter as it impacts both our teams. Metadata files are generated by otter and therefore their format needs to be decided with you and us working together. This cms.json file is not a metadata file and it has no influence on otter.

I would also remind that for the functional content metadata files that are not generated/handled by otter, when it was time to decide the format you literally told us "We don't handle these files so you do what you want", and you never reviewed their format, my team decided it alone.

Yes, this is not something, for my point of view, that should not have been accepted in a file hosted on this repository, mainly because it is a concept of a single product. This is an example of something we should not do so we can use it to continue to include things valid only for a single app or maybe to rethink the way the specification is managed. I would personally go for the second option.

As I said this schema file is in your repository for convenience for us and for the otter users that are integrated in AEM. In a perfect world it should be in a repo handled only by our team, with only us reviewing its content. We agreed on having this file in your repo some time ago after some discussions so that it is easier for my team, but we actually don't expect your team to review it. Comments and suggestions are welcome but in the end this file is our responsibility only.

Not sure to see why it would justify to have an option for a single product/app. If tomorrow another product will come with another option specific I don't see how you will check/document that this options is not for others.

Same as before, this is not a flag for a single app, this is a flag to configure the way the plugin works. Any app can use it and beneficiate from it if it does the necessary subsequent implementation that comes with it (using otter or not).

This is linked to the previous points, having options dedicated to the AEM Plugin makes sense, this is also the goal of Otter to facilitate the integration of applications within AEM Plugin and DGP Hosting;

In this aspect it would have been nice to have otter handling functional contents at its core the same way as it is done for other content types. But unfortunately your team refused to handle it, so here we are, having to put input flags to handle the feature, and forcing all the applications that want to use it to implement custom rule actions themselves.

but this is valid only if the options fit (and can be used in) all Otter based application. Here it is not the case definitively.

As AEM DES PLUGIN we do our best to not be linked to otter, and until now we succeeded. We can handle applications that do not use otter, or even do not use angular. Our scope is to be open to all applications (of course including otter applications that represent 100% of our clients today).
All the options present in this cms.json file can be used by any application, using otter or not.

For this case specifically, as explained in my previous comment, it is linked to a feature of Otter that enables custom action handlers, we can have a plenty of custom handler in different application, so it is really dommage to have a dedicated option for a dedicated implementation for one product/application of a specific action type.

You can have as many implementations as you want of custom handler, today the AEM DES PLUGIN only supports one. The one that has been specified by our team and that is activated in the plugin via this new flag. So there is nothing dommage here because again this flag is not specific to a particular application, it is specific to the plugin. Any application that wants to have functional content rule actions in the CMS has to follow and implement what has been specified by us (of course using the technical possibility that otter provides in its rules engine for applications using otter).

Copy link
Contributor

@kpanot kpanot Dec 4, 2024

Choose a reason for hiding this comment

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

Globally this can be summarized to a simple issue: we cannot have in Otter repo a file with notions/concept that are not explained (and discussed/agreed on).

So I can see only few mitigations:

  1. If you consider, as it seems to be, that this file is not relative to Otter at all; then you will need to find a way to relocate it outside of this repo, update app generator and provide migration scripts following guidelines (= ng-update).
  2. We agree that this file is an in-between, it continues to be hosted in Otter repository but it cannot include notion not part/documented of Otter. Which can be solved by more genericity in the properties name/description and/or documentation.

packages/@o3r/application/schemas/cms.schema.json Outdated Show resolved Hide resolved
@GCavailles GCavailles force-pushed the feature/schema-cms-update branch from fc53205 to 784afe4 Compare December 3, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants