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

Feature/#777 #783

Merged
merged 8 commits into from
Oct 9, 2023
Merged

Feature/#777 #783

merged 8 commits into from
Oct 9, 2023

Conversation

schoicsiro
Copy link
Contributor

Would you be able to get PR?

@schoicsiro schoicsiro changed the base branch from grails4 to dev September 13, 2023 06:29
@schoicsiro schoicsiro requested a review from temi September 13, 2023 06:52
@@ -34,6 +34,7 @@ class ApiController extends BaseController {
MapService mapService
ApiService apiService

@RequireApiKey()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is requireapikey required? Similarly other below actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previous ApiController class had @RequireApiKey. so I put it into all method except api/opus/list

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep security standard for all actions.

@@ -106,6 +107,41 @@ class ApiController extends BaseController {
}
}

@Path("/api/opus/list")
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to /api/opus for consistency with other APIs

@@ -106,6 +107,41 @@ class ApiController extends BaseController {
}
}

@Path("/api/opus/list")
@Operation(
summary = "Get all collection (opus) details",
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps better to reword it to Get all public collections

]
)
def getListCollections () {
List opus = profileService.getOpusList() as List
Copy link
Contributor

Choose a reason for hiding this comment

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

Reuse profileService.getOpus(). No need to implement a custom code in profile-hub or profile-service.

@@ -26,6 +26,8 @@ class ApiInterceptor {
Class controllerClass = controller?.clazz
def method = controllerClass?.getMethod(actionName, [] as Class[])

if (method.name == "getListCollections") return true
Copy link
Contributor

Choose a reason for hiding this comment

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

should not escape api interceptor logic.

@@ -45,6 +45,11 @@ class ProfileService {
webServiceWrapperService.get("${grailsApplication.config.profile.service.url}/opus/${encPath(opusId)}", [:], ContentType.APPLICATION_JSON, true, false, getCustomHeaderWithUserId())?.resp
}

def getOpusList() {
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above.

@@ -263,6 +263,7 @@ class UrlMappings {
get "/opus/$opusId/profile/$profileId/image" (version: "1.0", controller: "api", action: "getImages", namespace: "v1")
get "/opus/$opusId/profile/$profileId/attribute/$attributeId" (version: "1.0", controller: "api", action: "getAttributes", namespace: "v1")
get "/opus/$opusId/profile/$profileId/draft" (version: "1.0", controller: "api", action: "getDraftProfile", namespace: "v1")
get "/opus/list" (version: "1.0", controller: "api", action: "getListCollections", namespace: "v1")
Copy link
Contributor

Choose a reason for hiding this comment

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

should be /api/opus

@@ -333,6 +370,7 @@ class ApiController extends BaseController {
}
}

@RequireApiKey()
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove. applies to other implementations.

@schoicsiro schoicsiro requested a review from temi September 14, 2023 23:47
@@ -0,0 +1,12 @@
package au.org.ala.profile.domain
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this class to src/main/groovy/au/org/ala/profile/api/.

mediaType = "application/json",
array = @ArraySchema(
schema = @Schema(
implementation = OpusResponse.class
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with CollectionList since OpusResponse has a lost more properties than what is returned by this API.

@@ -1,4 +1,4 @@
package au.org.ala.profile.domain
package au.org.ala.profile.api
Copy link
Contributor

Choose a reason for hiding this comment

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

The location is not correct. Should not place this file with controllers. See comment before to get exact location.

@schoicsiro schoicsiro merged commit 56e4762 into dev Oct 9, 2023
1 check passed
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.

2 participants