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

PDF: support document description in request header #268

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

Conversation

chgeo
Copy link

@chgeo chgeo commented Jul 18, 2023

Add DocumentDescriptionLocation to support this scenario.

Also, make DocumentDescriptionReference and DocumentDescriptionCollection optional.

Add `DocumentDescriptionLocation` to support this scenario.

Also, make `DocumentDescriptionReference` and `DocumentDescriptionCollection`
optional.
@cla-assistant
Copy link

cla-assistant bot commented Jul 18, 2023

CLA assistant check
All committers have signed the CLA.

@@ -40,11 +40,17 @@
</Term>

<ComplexType Name="FeaturesType">
<Property Name="DocumentDescriptionReference" Type="Edm.String" Nullable="false">
<Property Name="DocumentDescriptionLocation" Type="PDF.DocumentDescriptionLocation" DefaultValue="0">
Copy link
Author

Choose a reason for hiding this comment

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

Not sure here if value 0 is correct. Would rather like to read it entitySet.

Choose a reason for hiding this comment

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

If this points to an enum, don't we need a value help for it?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't seen value helps in the other documents either, so would guess this is not a use case here.

For concrete DocumentDescription... properties, they are not user-facing, but only filled programmatically. One application, for example, will set DocumentDescriptionLocation to header, the other will set it to entitySet. But it's not the user that decides it.

Copy link
Contributor

@HeikoTheissen HeikoTheissen left a comment

Choose a reason for hiding this comment

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

Avoid property DocumentDescriptionLocation

@@ -40,11 +40,17 @@
</Term>

<ComplexType Name="FeaturesType">
<Property Name="DocumentDescriptionReference" Type="Edm.String" Nullable="false">
<Property Name="DocumentDescriptionLocation" Type="PDF.DocumentDescriptionLocation" DefaultValue="0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Property Name="DocumentDescriptionLocation" Type="PDF.DocumentDescriptionLocation" DefaultValue="0">

Comment on lines +53 to 55
<Property Name="DocumentDescriptionCollection" Type="Edm.String">
<Annotation Term="Core.Description" String="Name of entity set containing the DocumentDescription" />
</Property>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Property Name="DocumentDescriptionCollection" Type="Edm.String">
<Annotation Term="Core.Description" String="Name of entity set containing the DocumentDescription" />
</Property>
<Property Name="DocumentDescriptionCollection" Type="Edm.String">
<Annotation Term="Core.Description" String="Name of entity set containing the DocumentDescription" />
<Annotation Term="Core.LongDescription">
<String>If this is null, the request header 'SAP-Document-Description' must contain the base64-encoded JSON string of the document description.</String>
</Annotation>
</Property>

Comment on lines +109 to +116
<EnumType Name="DocumentDescriptionLocation">
<Member Name="entitySet" Value="0">
<Annotation Term="Core.Description" String="Document description is given as entity set" />
</Member>
<Member Name="header" Value="1">
<Annotation Term="Core.Description" String="Document description is given in a request header" />
</Member>
</EnumType>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<EnumType Name="DocumentDescriptionLocation">
<Member Name="entitySet" Value="0">
<Annotation Term="Core.Description" String="Document description is given as entity set" />
</Member>
<Member Name="header" Value="1">
<Annotation Term="Core.Description" String="Document description is given in a request header" />
</Member>
</EnumType>

@chgeo
Copy link
Author

chgeo commented Jul 25, 2023

Avoid property DocumentDescriptionLocation

Maybe I missed your point, but what do you mean by 'avoid DocumentDescriptionLocation'? It's the very thing I wanted to introduce.

@HeikoTheissen
Copy link
Contributor

Avoid property DocumentDescriptionLocation

Maybe I missed your point, but what do you mean by 'avoid DocumentDescriptionLocation'? It's the very thing I wanted to introduce.

I think we don't need it if we simply state

If DocumentDescriptionCollection is null, the request header 'SAP-Document-Description' must contain the base64-encoded JSON string of the document description.

@fb0d3nheimer
Copy link
Contributor

I think we don't need it if we simply state

This is too implicit. We need an explicit parameter for clean design. According to our architect.

@HeikoTheissen
Copy link
Contributor

This is too implicit. We need an explicit parameter for clean design. According to our architect.

I consider such dependencies between different properties unclean. Better have one complex type FeaturesHeaderType without DocumentDescriptionReference and DocumentDescriptionCollection and a derived complex type FeaturesEntitySetType that includes them (non-nullable). By choosing one of these two types, the annotation declares whether the document description is in the header or in the entity set.

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.

4 participants