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

Localized & draft & ID_texts #677

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Localized & draft & ID_texts #677

wants to merge 6 commits into from

Conversation

stewsk
Copy link
Contributor

@stewsk stewsk commented Feb 8, 2024

Document current behavior, reasoning, potential way forward ...

Document current behavior, reasoning, potential way forward ...
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Spelling Mistakes

  • ./advanced/fiori.md:497:77 Unknown word "redeploment"

Generally, for each spelling mistake there are 2 ways to fix it:

  1. Fix the spelling mistake and commit it.
  2. The word is incorrectly reported as misspelled → put the word on the project-words.txt list, located in the root project directory.

advanced/fiori.md Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@stewsk
Copy link
Contributor Author

stewsk commented Feb 8, 2024

@David-Kunz @beckermarc - as discussed in spec meeting on Monday: please add whatever you think is helpful, in particular w.r.t. runtime and Fiori.
Feel free to do with the existing text whatever you think is necessary :-)

Comment on lines 500 to 502
Today Fiori can also deal with composite keys with non-UUID types. For an entity with such a key,
Fiori cannot generate an initial value. So when creating a new entry, a pop-up is launched asking
the user to provide the key values.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather say that the backend doesn't allow key changes, hence Fiori Elements shows a popup to set the value once and doesn't allow to modify the value afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please add that to the text / change the text accordingly

@stewsk
Copy link
Contributor Author

stewsk commented Feb 15, 2024

@David-Kunz @beckermarc - little reminder - as discussed in spec meeting last week: please add whatever you think is helpful, in particular w.r.t. runtime and Fiori.
Feel free to do with the existing text whatever you think is necessary :-)

@agoerler agoerler requested a review from beckermarc March 12, 2024 11:01
advanced/fiori.md Outdated Show resolved Hide resolved
on by also adding the annotation `@fiori.draft.enabled` to the main entity.
This annotation does however not only include the `.texts` entity into the draft tree, but it also
changes the key of the `.texts`entity.
At the time draft support was implemented in CAP, Fiori required (TODO is this true?)
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is true

Fiori cannot generate an initial value. So when creating a new entry, a pop-up is launched asking
the user to provide the key values.

Is there a way to avoid this popup, but on the other hand keep the structure of the `.texts` entity
Copy link
Contributor

Choose a reason for hiding this comment

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

it is possible via annotation to suppress the popup (don't ask me what it is). node backend generates values for all uuid key properties, regardless how many. so it might be possible to just prompt the user for the locale info...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's possible to suppress the pop up for non-UUID keys. It's something, the user cannot change, even in draft mode, hence the moment the user 'hops out' of the field, it would need to become immutable - Fiori elements can't do that.

Is there a way to avoid this popup, but on the other hand keep the structure of the `.texts` entity
and table unchanged?

The basic idea is to have in the CDS runtime and for the database a `.texts` entity and table with
Copy link
Contributor

Choose a reason for hiding this comment

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

so it would be the following?

CREATE TABLE S_Books_texts (
  locale NVARCHAR(14) NOT NULL,
  ID INTEGER NOT NULL,
  title NVARCHAR(255),
  PRIMARY KEY(ID, locale)
);

Is there a way to avoid this popup, but on the other hand keep the structure of the `.texts` entity
and table unchanged?

The basic idea is to have in the CDS runtime and for the database a `.texts` entity and table with
Copy link
Contributor

Choose a reason for hiding this comment

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

the basic idea for what?

This only works, however, if we can guarantee that Fiori never sends a query based on `ID_texts`.
So, an ObjectPage for a text entry would not work.


Copy link
Contributor

Choose a reason for hiding this comment

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

issue: when creating the new instance, we need to generate the id in the backend. at that point, however, we don't yet know the intended locale -> anything that tries to concatenate a fake id field on read (e.g., ID + '_' + locale as ID_texts) is doomed.

Copy link
Contributor

Choose a reason for hiding this comment

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

anything that tries to concatenate a fake id field on read (e.g., ID + '_' + locale as ID_texts) is doomed.

Not during runtime, only when doing the CSV import, so it should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

In CAP Java CSVs are also uploaded by the runtime (in case of in-memory DBs). We chould have a well-defined hashing algorithm, which is used accross the runtimes.

Co-authored-by: Adrian Görler <[email protected]>
advanced/fiori.md Outdated Show resolved Hide resolved
advanced/fiori.md Outdated Show resolved Hide resolved
Comment on lines +443 to +447
At the time draft support was implemented in CAP, Fiori required
that a draft enabled entity must have a single key element of type UUID.
As a `.texts` entity by construction has at least two key elements (the `locale` plus the key elements
of the main entity), we remove the `key` property from all these elements and add a technical key element
`ID_texts` of type UUID.
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the motivation to introduce ID_texts was to allow to change the locale of an inactive draft which would not be possible if the locale was part of the key. Otherwise I don't think the compound key would be an issue.

@beckermarc - correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this was the reason. Draft itself worked with keys locale plus the original keys, but the problem was that the locale had to be given at creation time of the new text and couldn't be changed anymore afterwards. This was undesirable from UX perspective.

Comment on lines +495 to +498
Deployment (for PostgreSQL) has recently been improved to handle this. For each entry, the values of
the original key elements are used to generate a hash value which is then used as value for
`ID_texts`. It is important the the generated value is stable, so that upon redeployment the same values
for `ID_texts` are produced.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was not aware of this. I assume that the generated hash value adheres to the UUID format.

Maybe we could invent an @cds.on.insert-variant to express that a hash should be computed from the data upon insert.

This only works, however, if we can guarantee that Fiori never sends a query based on `ID_texts`.
So, an ObjectPage for a text entry would not work.


Copy link
Contributor

Choose a reason for hiding this comment

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

In CAP Java CSVs are also uploaded by the runtime (in case of in-memory DBs). We chould have a well-defined hashing algorithm, which is used accross the runtimes.

@@ -425,6 +425,97 @@ Adding the annotation `@fiori.draft.enabled` won't work if the corresponding `_t

If you're editing data in multiple languages, the _General_ tab in the example above is reserved for the default language (often "en"). Any change to other languages has to be done in the _Translations_ tab, where a corresponding language can be chosen from a drop-down menu as illustrated above. This also applies if you use the URL parameter `sap-language` on the draft page.

#### Background

> TODO: find a good place for this section, maybe make it internal
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

5 participants