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

V14/feature/response model type info improvements #14962

Closed

Conversation

Migaroez
Copy link
Contributor

@Migaroez Migaroez commented Oct 12, 2023

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes

Description

Added a type property to all ResponseModels that return actionable objects, most of them are entitites.
This helps the frontend to determine what UI they need to show when an intent for manipulation on these items is requested (especially in mixed lists).

Also

  • made some ResponseModels more specific
  • made all type fields fixed
  • updated some controllers to return ResponseModels instead of more general Presentation models.

Possible improvements

For consistency, we are reusing entityUdi keys for most of the types. But we have to introduce some new values and it might not be the correct place to do so.

Reproduction

All fetch endpoints for the following resources should supply a type key that is the same for all endpoints within that resource

  • Dictionary
  • Document (seperate type for items requested trough the recyblebin endpoints)
  • Media (seperate type for items requested trough the recyblebin endpoints)
  • Membergroup
  • MemberType
  • Partialview
  • Relation
  • RelationType
  • Script
  • StaticFile
  • Stylesheet
  • Template
  • Language
  • User

@Migaroez Migaroez marked this pull request as ready for review October 12, 2023 10:49
Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

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

Hi @Migaroez 👋

A few comments from my part:

  1. The new UDI type constants might be somewhat questionable. In general, a UDI type constant should only exist if an entity is uniquely identifiable by said UDI type -- that is, can be queried and/or identified in code (not in human eyes). For example, a document in the recycle bin will still be identified by the Document UDI type.
  2. A lot of models need a linebreak between the existing properties and the Type property override.
  3. A few changes are exclusively due to added and unused usings. I have noted FolderResponseModel and ContentTreeItemResponseModel, there may be others.

@Migaroez
Copy link
Contributor Author

Migaroez commented Oct 17, 2023

Thx @kjac for the critical eye!

2 & 3 have been taken care off.

Regarding point one, I had a different approach previously where i proxied the udis as getOnly strings in a static class combined with constant strings for the non udi types and called it ResourceObjectTypes in the ...Api.Management.Constants namespace.
Advantage was to keep the naming consistent across UDI and resource types while being able to add types that didn't map onto UDI types
But this led some namespace conflicts with introducing a Constants namespace and this is why I abandoned the idea. Any suggestions besides moving the constants namespace under ...Api.Management.ViewModels?

@Migaroez
Copy link
Contributor Author

Responsibility was moved to frontend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants