SAP / open-ux-tools

Enable community collaboration to jointly promote and facilitate best in class tooling capabilities
Apache License 2.0
83 stars 33 forks source link

FEATURE - Automatically enable the notes reuse component if a service is annotated with UI.Note #1924

Open tobiasqueck opened 3 months ago

tobiasqueck commented 3 months ago

As a developer, I want to generate a Fiori elements for OData v4 application for a service with the UI.Notes annotation that is working out of the box, so that I can work with it without having to manually add the reuse component

Description

In 2023 the new UI.Note annotation (https://github.com/SAP/odata-vocabularies/pull/221/files) was introduced. In addition, in the latest SAP systems, there is a reuse component sap.nw.core.gbt.notes.lib.reuse designed to be used if the annotation UI.Note is part of the service metadata. For a Fiori elements app to be able to make user of the reuse lib, it needs to be added as a dependency to the manifest. Since, we know the metadata (and annotation) at generation time, it would be great if the reuse lib is added automatically if the annotation is present.

Example (mocked) metadata for testing: metadata.xml.zip.

Technical Design

It is only relevant for the fiori-elements-writer module specifically for apps with an OData v4 service. So, in the module, if version is 4 then we should check if the provided metadata contains the UI.Note annotation. If yes, then we add nw.core.gbt.notes.lib.reuse as a dependency to the manifest.json, so that it looks like below for a Fiori elements for OData v4 app

"sap.ui5": {
    "dependencies": {
        "libs": {
            "sap.fe.templates": {},
            "sap.nw.core.gbt.notes.lib.reuse": {}
        }
    }
}

I would recommend to keep it simple, and do a check of the metadata (string) for @UI.Note. if included, at the lib, if not then don't. You can (but again, I wouldn't suggest it), do a more precise check and see if the annotations contain a UI.ReferenceFacet with a target pointing to the annotation.

Acceptance Criteria

GivenWhenThen format (https://martinfowler.com/bliki/GivenWhenThen.html)

Given I have a v4 service with the UI.Note annotation... when I generate a Fiori elements (including FPM) app then the generated manifest will contain the required dependency.

Given I have a v4 service WITHOUT the UI.Note annotation... when I generate a Fiori elements (including FPM) app then the generated manifest will NOT contain the dependency.

Notes

Tasks

IainSAP commented 3 months ago

@tobiasqueck Are we sure this code should go to the writer, have you considered future addition of more annotation/libs? . The generator calling the writer will already have parsed the annotation file to determine entities and so perhaps thats where the logic should live rather than in the writer itself. Otherwise now we will need to validate the xml structure etc here and introduce a parser. Im not sure this is the concern of the writer and setting a precedent (contents of annotation file determine writer output) should be done carefully as in the end it will grow.

tobiasqueck commented 3 months ago

@IainSAP my proposal is based on the thought that the annotations are given to the writer, therefore, the writer can make the decision because it needs nothing else. So, it didn't make sense to me to do it beforehand. I understand your argument about the parsing (i.e. having to do it twice), however, logically it would only fit into the prompting if we allow the user to make a choice about the libs. So, especially, if we have more libs in the future based on the provided input (e.g. annotations), I'd expect them in the writer.

Nevertheless, I am not "too stubborn" about that one, if you prefer doing it in the prompting phase and enhance the writer to allow adding a libs, then I am also ok.

IainSAP commented 3 months ago

@tobiasqueck Thanks I see how your proposal adds value actually, if you only use the writer it would be simpler to have it also take care of determining the output based on the annotations. I think thats reasonable. We will do it (in the writer) in a nicely extensible way of course so its straightforward to add additional annotation based libs in future. The ui5-application-writer already supports adding libs. Thx!

korotkovao commented 2 months ago

Hello @tobiasqueck ,

  1. I have found an internal wiki info that mentions sap.nw.core.gbt.notes.lib.reuse Could you kindly confirm what entries we should add to the manifest, please? Is sap.nw.core.gbt.notes.lib.reuse added under components{} in sap.ui5:{ dependencies: { components: { or somewhere else ?

  2. Do we assume sap.nw.core.gbt.notes.lib.reuse is always available at the frontend server? Or should we check whether this reuse lib is available before we add it to the manifest when creating a v4 project?

  3. How do we check this functionality once implemented?

@devinea

tobiasqueck commented 2 months ago

@korotkovao

  1. This is what a sap.fe app needs if the notes component is required
    "sap.ui5": {
    "dependencies": {
        "libs": {
            "sap.fe.templates": {},
            "sap.nw.core.gbt.notes.lib.reuse": {}
        }
    }
    }
  2. If the annotation is present in a service, then we add it. Fiori elements at runtime handles the case of the lib not being available despite the annotation
  3. I have an internal test app that I can send you - I cannot share it here because it is internal.
korotkovao commented 1 month ago

@tobiasqueck

I have tested a few configurations and thought I'd document my findings here.

Case A, VSCode and metadata generated app With an app generated in VSCode with a metadata file, "sap.nw.core.gbt.notes.lib.reuse": {} in the manifest.json and no backend destination config, start-local loads with errors image

With an app generated in VSCode with a metadata file, "sap.nw.core.gbt.notes.lib.reuse": {"lazy": true} in the manifest.json and no backend destination config, start-local loads, however, please observe the Note dependency couldn't be loaded. image

The only way around is to add the backend config + an entry to index.html where reuse library can be loaded from image

Note dependency loaded with start-local image


Case B, BAS and service generated app "sap.nw.core.gbt.notes.lib.reuse": {"lazy": true} in the manifest.json and no backend destination config, start-local loads Note dependency loaded with start-local image

tobiasqueck commented 1 month ago

Great findings!

Please mark all reuse libs as {"lazy": true} in the manifest. That should be best practice. About the index.html, the path is rather static, so we could add it during generation. And, if someone has the lib at a different path, then the can always change it later.