-
Notifications
You must be signed in to change notification settings - Fork 2
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
clark-service-refactor
to main
#1693
Conversation
.husky/pre-commit
Outdated
@@ -1,4 +1,4 @@ | |||
#!/bin/sh | |||
. "$(dirname "$0")/_/husky.sh" | |||
|
|||
npm run lint && npm run test:unit | |||
npm run lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would run prettier fix here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good looks
"lint": "npx ng lint clark", | ||
"lint:fix": "npx ng lint clark --fix", | ||
"prettier": "prettier --check .", | ||
"e2e": "ng e2e", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a script for prettier:fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have two cores? One under src/app/admin/core and on under src/app/core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way we purpose ../core/..
is to support any Angular service or guard, in addition to a few client supporting components. The unique part of and *.service.ts
file in a core is not always to interact with our API, it's a client service that controls a particular component, for an example. That is the case here in admin/core
where most of these service files are controlling logic for client interactions.
To fully answer your question, we should refactor to having one core within src/app
however, I argue that moving every existing core to one core is a refactor out of scope of route updates for clark-service
await this.http | ||
.request( | ||
ACCESS_GROUP_ROUTES.REMOVE_ACCESS_GROUP_FROM_USER(memberId), | ||
collection, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This route won't work. The Dto for this route is expecting an accessGroup to remove from the user.
@@ -151,17 +118,14 @@ export class UserService { | |||
* @memberof UserService | |||
*/ | |||
getOrganizationMembers(organization: string): Promise<User[]> { | |||
const route = USER_ROUTES.GET_SAME_ORGANIZATION(organization); | |||
const route = LEGACY_USER_ROUTES.GET_SAME_ORGANIZATION(organization); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used? Should we be adding this to clark-service?
// * | ||
// * @returns An observable containing an array of blogs | ||
// */ | ||
// async getRecentBlogs(): Observable<Blog[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this is commented out?
Clark-service supports the query recent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch on mfranz1 branch
Clark-service needs to be updated to look at query param not body
* @returns {Promise<void>} | ||
* @memberof UserService | ||
*/ | ||
async removeMember(collectionName: string, memberId: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function isn't being referenced anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few comments and questions.
.circleci/config.yml
Outdated
- releases | ||
- cyber4all/s3: | ||
context: [AWS, Slack] | ||
bucket: "clark-prod-file-uploads" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this the production upload bucket?
@@ -89,7 +89,7 @@ export class AdminUserCardComponent { | |||
} | |||
|
|||
async canAddEvaluator() { | |||
const roles = await this.privilegeService.getCollectionRoles(this.user.id); | |||
const roles = await this.accessGroupsService.getUserAccessGroups(this.user.username); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not using id?
@@ -73,7 +73,6 @@ export class UserSearchWrapperComponent implements OnInit, OnDestroy { | |||
this.loading = false; | |||
}).catch(error => { | |||
this.toaster.error('Error!', 'There was an error fetching users. Please try again later.'); | |||
console.error(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for cleaning these up.
}) | ||
.catch(error => { | ||
this.toaster.error('Error!', 'Could not remove reviewer. Please try again later'); | ||
console.log(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
private ngUnsubscribe = new Subject<void>(); | ||
ssoRedirect = environment.apiURL + '/google'; | ||
ssoRedirect = AUTH_ROUTES.GOOGLE_SIGNUP(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
c.hasLogo = true; | ||
}); | ||
} catch (error) { | ||
console.log(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
@@ -130,22 +133,22 @@ export class FeaturedObjectsService { | |||
* @returns {Promise<LearningObject[]>} | |||
* @memberof LearningObjectService | |||
*/ | |||
getNotFeaturedLearningObjects(query?: Query): Promise<{learningObjects: LearningObject[], total: number}> { | |||
getNotFeaturedLearningObjects(query?: Query): Promise<{ learningObjects: LearningObject[], total: number }> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used on the featured admin page. It gets the learning objects that aren't featured.
@@ -20,9 +22,10 @@ export class HttpConfigInterceptor implements HttpInterceptor { | |||
request: HttpRequest<any>, | |||
next: HttpHandler | |||
): Observable<HttpEvent<any>> { | |||
console.log('intercepted request ... '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
@@ -113,7 +116,7 @@ export class LibraryService { | |||
// Show loading spinner | |||
this._loading$.next(true); | |||
// Url route for bundling | |||
const bundle = USER_ROUTES.OBJECT_BUNDLE( | |||
const bundle = BUNDLING_ROUTES.BUNDLE_LEARNING_OBJECT( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
History: Library used to have a download button and we used to bundle everything on the fly.
* Request to get all learning object stats | ||
* @method GET | ||
*/ | ||
GET_LEARNING_OBJECT_STATS() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably related to the weird 2 stats routes we had in library and learning object service.
not needed. flags can be deleted from database directly Signed-off-by: Andreas <andreas.papacharalampous@secured.team>
Signed-off-by: Andreas <andreas.papacharalampous@secured.team>
…le-route-updates-for-clark-refactor
This reverts commit 237ade8.
…OBJECTS_WITH_LIMIT. Updated function parameter name for GET_COLLECTION_FEATURED_OBJECTS in featured.service.ts. Added types to function parameters
…odule-route-updates-for-clark-refactor
…g-in-curator-dashboard-always fix: [sc-32644] Curator - Filtering in curator dashboard always returns all released learning objects
Signed-off-by: Andreas <andreas.papacharalampous@secured.team>
…-module-route-updates-for-clark update access group functions
…g-objects bug fixes clark-service deployment
…g-objects Bug/sc 35478/browse 0 learning objects
…rk-client into admin-search-users
Admin search users
Admin search users
Admin search users
Admin search users
…api-env-for-api-clark-center Feature/sc 35546/swaping the api env for api clark center
This PR updates
clark-client
to conform to changes in clark-services. All services have been rewritten except forlearning-object-service
. All LOS routes are still currently being used to the old service and are referred to as legacy routes in the client.Majority of file changes are files that have been reorganized into subdirectories of
src/app/core
. The core module now reflects our clark-service/CLARK-Gateway strutcures.This pull request is linked to Shortcut Story #28112: Update clark-client to reflect changes in clark-service.