Skip to content
This repository has been archived by the owner on Dec 13, 2020. It is now read-only.

frontend: change view attributes endpoint location #783

Closed
teosarca opened this issue May 23, 2017 · 5 comments
Closed

frontend: change view attributes endpoint location #783

teosarca opened this issue May 23, 2017 · 5 comments
Assignees
Milestone

Comments

@teosarca
Copy link
Member

What is the current behavior?

Current view attributes endpoint is /rest/api/documentView/{windowId}/{viewId}/{rowId}

What is the expected or desired behavior?

Change it to: /rest/api/documentView/{windowId}/{viewId}/{rowId}/attributes
i.e. we will append the "/attributes" because we want to avoid overlapping with other endpoints will be based on current view row (/rest/api/documentView/{windowId}/{viewId}/{rowId}).

Which are the steps to reproduce?

Browse some HUs in https://w101.metasfresh.com:8443/window/540189
Mind the attributes related endpoint calls.

image

Backend task: metasfresh/metasfresh-webui-api-legacy#409

@damianprzygodzki
Copy link
Contributor

getData with attributes is understandable (because we are getting attributes) but we need then to populate to every getData endpoint (window, process, etc)

patch is also okay, we are patching changes for attributes (but we have to populate also to other entities)

dropdown and typeahead are totally wrong, /attributes/attribute is misleading. if we want to attributes we should get rid of attribute, so:

  • …/{rowId}/attributes/{attributeName}/…
  • …/{rowId}/attribute/{attrName}/…
    But also, we have to propagate to every attribute endpoint

layout should be as it was, because we are getting layout for docView, not layout for attributes

And to clarify, i don’t want to evaluate your changes and ideas. I just think that we created very nice protocol, and dont want to make step backward. Of course that i can add everywhere hardocded prefix, but our schema now is handling every place in application! So it is really nice.

@teosarca
Copy link
Member Author

teosarca commented May 24, 2017

layout should be as it was, because we are getting layout for docView, not layout for attributes

If we are talking about GET /rest/api/documentView/{windowId}/{viewId}/{rowId}/layout then we are talking about attributes layout.

And basically the whole REST controller ( http://w101.metasfresh.com:8081/swagger-ui.html#/view-row-attributes-rest-controller-_-old ) is offering services around attributes.

So the only change that has to be done is to change the view row attribute's root endpoint
from: /rest/api/documentView/{windowId}/{viewId}/{rowId}
to: /rest/api/documentView/{windowId}/{viewId}/{rowId}/attributes

Because,

  • the old root is misleading anyways, i.e. it suggesting that it's about a view row but it's not, it's about view row's attributes
  • prevents us from adding more services around a given row

About the attribute specific endpoints (/rest/api/documentView/{windowId}/{viewId}/{rowId}/attribute/{attributeName}/dropdown and typeahead),
i was considering the following:

  1. /rest/api/documentView/{windowId}/{viewId}/{rowId}/attributes/attribute/{attributeName}/dropdown => proposed solution which is homogenous with the other attributes related services (i.e. they all share the same root)

  2. /rest/api/documentView/{windowId}/{viewId}/{rowId}/attribute/{attributeName}
    => not homogenous
    => might involve more changes on frontend side (can u confirm?)

  3. /rest/api/documentView/{windowId}/{viewId}/{rowId}/attributes/{attributeName}/dropdown
    => looks nice but as soon as somebody defines the an attribute called "layout" for example, the /layout endpoint won't work anymore because it is basically overlapping with the "layout" attribute name

In conclusion, we either go with this approach or we come up with a new solution but which addresses the issues which this task is supported to solve:

  • being able to hook up more services on /rest/api/documentView/{windowId}/{viewId}/{rowId} resource URL
  • attribute names shall not overlap with other "service" names (see the "layout" case)

@Dunkat
Copy link
Contributor

Dunkat commented May 26, 2017

Maybe we should consider if it's really needed to have /attributes everywhere because something like this /attributes/attribute might look not good.

@teosarca
Copy link
Member Author

Hi Kasia, i also don't like it but so far that's the only case.
In future if there will be needed in other places too, we might have to consider something like '/attributes/byName/MyAttrName'....

@teosarca
Copy link
Member Author

Btw, i believe everything was done for now (on other tasks), so i would close this ticket.

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

No branches or pull requests

3 participants