Moving Data Distribution API into the Vitro codebase #484
Conversation
|
@markuskotte agreed to review German language parts, when and if necessary. |
There was a problem hiding this comment.
@litvinovg thanks for nice and significant contribution. I have posted a couple of comments.
| log.debug("Setting CORS header for every request."); | ||
| resp.setHeader("Access-Control-Allow-Origin", "*"); |
There was a problem hiding this comment.
We might consider to offer some control over CORS, maybe it is not needed at the level of a single endpoint, but at least at the VIVO instance level. For instance, to have a runtime property defining value for Access-Control-Allow-Origin.
We don't have any control over access to an API endpoint at the moment
There was a problem hiding this comment.
Maybe it would even make sense to define that values for each distributor?
I am not even sure we have to set it to * for current use cases. Maybe just remove it for now?
There was a problem hiding this comment.
Let's remove this for now.
Also, please add access rights for API endpoints. The idea is to threat endpoint as a class or object and to offer via UI to define which role has access to that. Operation might be "execute", if it is too complicate to add new operation, we might use some already existing (e.g. "display")
There was a problem hiding this comment.
Also, please add access rights for API endpoints. The idea is to threat endpoint as a class or object and to offer via UI to define which role has access to that. Operation might be "execute", if it is too complicate to add new operation, we might use some already existing (e.g. "display")
@litvinovg this part has not beet addressed yet, right?
There was a problem hiding this comment.
Also, please add access rights for API endpoints. The idea is to threat endpoint as a class or object and to offer via UI to define which role has access to that. Operation might be "execute", if it is too complicate to add new operation, we might use some already existing (e.g. "display")
@litvinovg this part has not beet addressed yet, right?
Yes.
...u/cornell/library/scholars/webapp/controller/api/distribute/DataDistributionApi_overview.png
Outdated
Show resolved
Hide resolved
...ll/library/scholars/webapp/controller/api/distribute/rdf/graphbuilder/EmptyGraphBuilder.java
Outdated
Show resolved
Hide resolved
...library/scholars/webapp/controller/api/distribute/rdf/graphbuilder/ParallelGraphBuilder.java
Outdated
Show resolved
Hide resolved
...brary/scholars/webapp/controller/api/distribute/rdf/graphbuilder/ThreadsafeGraphBuilder.java
Outdated
Show resolved
Hide resolved
home/src/main/resources/rdf/i18n/en_US/interface-i18n/firsttime/vitro_UiLabel.ttl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Can you please also generate those labels for other languages?
There was a problem hiding this comment.
We agreed not to add other languages within this PR, we will do that for all new features in a release in one specific PR for this purpose.
| log.debug("Setting CORS header for every request."); | ||
| resp.setHeader("Access-Control-Allow-Origin", "*"); |
There was a problem hiding this comment.
Let's remove this for now.
Also, please add access rights for API endpoints. The idea is to threat endpoint as a class or object and to offer via UI to define which role has access to that. Operation might be "execute", if it is too complicate to add new operation, we might use some already existing (e.g. "display")
There was a problem hiding this comment.
We agreed not to add other languages within this PR, we will do that for all new features in a release in one specific PR for this purpose.
takeover from graham rm unimplemented or unnecessary classes rm unused distributors added us text for vqt use local server port to make new request instead of port used by the client fix self signed certificate issue Renamed german translation properties checkstyle fixes
This reverts commit 7c2f665991b94c8d9f7e4f38342502815a4be8f7.
This reverts commit 915f96dc8549f11aad44c62a21f5096e02345528.
…ge and not used translations
litvinovg
force-pushed
the
VIVO-4027+
branch
from
92dabdc to
301dfd1
Compare
VIVO GitHub issue
What does this pull request do?
Added the Data Distribution API and report generation into Vitro codebase.
Additional Notes:
Any additional information that you think would be helpful when reviewing this PR.
Interested parties
@chenejac @hauschke @brianjlowe
Reviewers' expertise
Candidates for reviewing this PR should have some of the following expertises:
Reviewers' report template
Please update the following template which should be used by reviewers.
General comment
A reviewer should provide here comments and suggestions for requested changes if any.
Testing
A reviewer should briefly describe here how it was tested
Code reviewing
A reviewer should briefly describe here which part was code reviewed