Add a Platform service #703

fgravin · 2023-11-24T21:09:34Z

Add a platform service interface + a gn4 implementation to centralize

user repository
auth
organization repository
api compatibility & version

Please read commit history for more information.
Only some parts of the code use this platform, mostly auth Observables, users and version things.

Incrementally, we could move the organizations repositories into it.

Code ready for a review.
Tests will come if it's ok.

github-actions · 2023-11-24T21:10:43Z

Affected libs: api-metadata-converter, feature-editor, api-repository, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, feature-auth, ui-search, common-domain, common-fixtures, ui-elements, ui-catalog, util-shared, ui-widgets, ui-inputs, ui-map, ui-dataviz,
Affected apps: data-platform, datahub, metadata-converter, metadata-editor, webcomponents, demo, search, map-viewer, datafeeder,

🚀 Build and deploy storybook and demo on GitHub Pages
📦 Build and push affected docker images

jahow

Thanks ✌️ I think this is going in the right direction. I would have liked the auth service to be gone completely though, but maybe this will take too much refactoring.

libs/common/domain/src/lib/platform.service.interface.ts

jahow · 2023-11-25T18:41:46Z

libs/api/repository/src/lib/gn4/platform/gn4-platform.service.ts

+  readonly isApiCompatible$ = this.apiVersion$.pipe(
+    tap(
+      (version) =>
+        version < minApiVersion &&


Does that really work for semver? e.g. 4.10.0 might be considered "lower" than 4.2.0

Yes this is not correct, I will add the test of the version in another PR and keep this one as just refactoring.
I don't want that some heavy work is started and brings conflicts because of this refactoring.

libs/common/domain/src/lib/model/user/user.model.ts

coveralls · 2023-11-27T15:22:09Z

coverage: 82.889% (-3.1%) from 85.955%
when pulling 548263b on platform-service
into ef6a273 on main.

jahow

I did a first pass on the changes, most of it looks OK but I still stumbled on some possible improvements. I feel like it would have been a great opportunity to get rid of the AuthService as well, but maybe that was taking the refactoring too far?

apps/datahub/src/app/home/home-header/home-header.component.spec.ts

apps/metadata-editor/src/app/app.module.ts

libs/api/repository/src/lib/gn4/auth/auth.service.spec.ts

libs/feature/search/src/lib/state/effects.spec.ts

libs/common/domain/src/lib/model/user/user.model.ts

libs/api/repository/src/lib/gn4/platform/gn4-platform.service.spec.ts

jahow · 2023-11-28T09:06:30Z

libs/api/repository/src/lib/gn4/gn4.provider.ts

+      provide: PlatformServiceInterface,
+      useClass: Gn4PlatformService,
+    },
+    Gn4PlatformMapper,


I don't see why this should be provided? Isn't it an explicit dependency of Gn4PlatformService?

Because it's not injected in root, so it has to be injected somehow.
There is no providers option from services.

jahow · 2023-11-28T09:07:09Z

libs/api/repository/src/lib/gn4/platform/gn4-platform.mapper.ts

+import { AvatarServiceInterface } from '../auth/avatar.service.interface'
+
+@Injectable()
+export class Gn4PlatformMapper {


I feel like these mappers could be simple pure functions exported as is, instead of an Angular service.

Yes I actually wanted to create a static class, but the mappers need for instance the AvatarService which is injected.

jahow · 2023-11-28T09:07:30Z

libs/api/repository/src/lib/gn4/platform/gn4-platform.mapper.ts

+      credentialsNonExpired,
+      ...user
+    } = apiUser
+    return { ...apiUser, id: id + '' } as UserModel


Suggested change

return { ...apiUser, id: id + '' } as UserModel

return { ...apiUser, id: id.toString() } as UserModel

Many repositories should be implemented.move record repository to this folder.

move all models to this folder

automatic refactoring created wrong path :/

remove code from AuthService and use the platform instead

make things clearer and move the User model in domain from gn4-api

which should be responsible of gathering all domain interface implementation for the gn4 strategy

move from AuthService to PlatformeService

to avoid using global subjects which could create side effects

jahow

Let's go!

fgravin added the refactoring label Nov 24, 2023

fgravin requested a review from jahow November 24, 2023 21:09

jahow reviewed Nov 25, 2023

View reviewed changes

fgravin requested a review from jahow November 27, 2023 21:19

fgravin force-pushed the platform-service branch from 0833d60 to b7bd72f Compare November 28, 2023 08:19

jahow reviewed Nov 28, 2023

View reviewed changes

fgravin added 12 commits November 28, 2023 13:52

refactor(domain): create a repository folder

eee4c44

Many repositories should be implemented.move record repository to this folder.

refactor(domain): create a model folder

3a71bcd

move all models to this folder

refactor(domain): move dataviz -configuration model to /model

1644a75

feat(domain): add a platform service interface

68418e2

refactor(domain): fix absolute path in imports

619b585

automatic refactoring created wrong path :/

refactor(repository): add gn4 platform service implementation

a71d063

refactor(repository): use Platform service in gn4 repository

b69ce36

remove code from AuthService and use the platform instead

refactor(user): differenciate User & MeUser models

8e46984

make things clearer and move the User model in domain from gn4-api

refactor(repository): create a gn4 provider

abe5f1b

which should be responsible of gathering all domain interface implementation for the gn4 strategy

test(platform): adapt new imports from tests

0ae61d8

move from AuthService to PlatformeService

refactor(user): merge MeUser & User models

4791add

refactor(platform): rework test to use inner Observables

548263b

to avoid using global subjects which could create side effects

fgravin force-pushed the platform-service branch from b7bd72f to 548263b Compare November 28, 2023 12:52

fgravin requested a review from jahow November 28, 2023 13:16

jahow approved these changes Nov 28, 2023

View reviewed changes

fgravin merged commit e7e9323 into main Nov 28, 2023
8 checks passed

fgravin deleted the platform-service branch November 28, 2023 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Platform service #703

Add a Platform service #703

fgravin commented Nov 24, 2023 •

edited

Loading

github-actions bot commented Nov 24, 2023 •

edited

Loading

jahow left a comment

jahow Nov 25, 2023

fgravin Nov 28, 2023

coveralls commented Nov 27, 2023 •

edited

Loading

jahow left a comment

jahow Nov 28, 2023

fgravin Nov 28, 2023

jahow Nov 28, 2023

fgravin Nov 28, 2023

jahow Nov 28, 2023

jahow left a comment

	return { ...apiUser, id: id + '' } as UserModel
	return { ...apiUser, id: id.toString() } as UserModel

Add a Platform service #703

Add a Platform service #703

Conversation

fgravin commented Nov 24, 2023 • edited Loading

github-actions bot commented Nov 24, 2023 • edited Loading

jahow left a comment

Choose a reason for hiding this comment

jahow Nov 25, 2023

Choose a reason for hiding this comment

fgravin Nov 28, 2023

Choose a reason for hiding this comment

coveralls commented Nov 27, 2023 • edited Loading

jahow left a comment

Choose a reason for hiding this comment

jahow Nov 28, 2023

Choose a reason for hiding this comment

fgravin Nov 28, 2023

Choose a reason for hiding this comment

jahow Nov 28, 2023

Choose a reason for hiding this comment

fgravin Nov 28, 2023

Choose a reason for hiding this comment

jahow Nov 28, 2023

Choose a reason for hiding this comment

jahow left a comment

Choose a reason for hiding this comment

fgravin commented Nov 24, 2023 •

edited

Loading

github-actions bot commented Nov 24, 2023 •

edited

Loading

coveralls commented Nov 27, 2023 •

edited

Loading