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

PIN-5555 eservice delegateClientAccess #1193

Merged

Conversation

paologaleotti
Copy link
Collaborator

@paologaleotti paologaleotti commented Nov 12, 2024

Don't merge before #1188

POST eservice:
immagine

GET Eservice:
immagine

Eservice su mongo:
immagine

update eservice:
immagine

GET con flag updated:
immagine

producer eservice:
immagine

@paologaleotti paologaleotti changed the title IMN-5555 delegateClientAccess IMN-5555 eservice delegateClientAccess Nov 12, 2024
@paologaleotti paologaleotti changed the title IMN-5555 eservice delegateClientAccess PIN-5555 eservice delegateClientAccess Nov 12, 2024
@MalpenZibo MalpenZibo force-pushed the feature/eservice-isdelegable-PIN-5554 branch from ffee648 to 0f7ec96 Compare November 12, 2024 10:57
Copy link
Collaborator

@ecamellini ecamellini left a comment

Choose a reason for hiding this comment

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

I left the same comments I left on #1188, furthermore for this PR I would investigate what happens in case of the two updates (eservice draft and after the publication of the eservice). Since the flag isDelegable is editable in both cases, we shall define what happens to this flag and why it shall not be editable if the other is editable

packages/api-clients/open-api/bffApi.yml Outdated Show resolved Hide resolved
packages/models/src/eservice/protobufConverterToV2.ts Outdated Show resolved Hide resolved
@MalpenZibo MalpenZibo force-pushed the feature/eservice-isdelegable-PIN-5554 branch from 2dad452 to 242a9f2 Compare November 18, 2024 10:25
Copy link
Collaborator

@MalpenZibo MalpenZibo left a comment

Choose a reason for hiding this comment

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

👍 just one super minor comment

packages/catalog-process/src/services/catalogService.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ecamellini ecamellini left a comment

Choose a reason for hiding this comment

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

Leaving the same comment I left in #1188 👇

Three things are still missing IMO:

  • Adding the flag into the various read calls in BFF and API GW (only the ones that don't return compact data)
  • Adding the flag into the eservice import/export in BFF (see fileUtils.ts)
  • Updating the corresponding Bruno calls, performing some manual tests, and adding some test screenshots to the PR

You can use #1109 as a reference for all the points, where I did the same for the isSignalHubEnabled flag.

packages/catalog-process/src/services/catalogService.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@ecamellini ecamellini left a comment

Choose a reason for hiding this comment

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

Looks good!
I would also add a couple of new tests to test the create/update cases where:

  • the new flag is set to some value X
  • the isDelegable flag is false/undefined
    In these cases we shall test that in the eservice the new flag is set to false/undefined, the value of isDelegable, and the value X from the seed is ignored

packages/catalog-process/test/updateEservice.test.ts Outdated Show resolved Hide resolved
Base automatically changed from feature/eservice-isdelegable-PIN-5554 to feature/incaricato November 26, 2024 09:20
@paologaleotti paologaleotti merged commit 63ae17f into feature/incaricato Nov 27, 2024
49 checks passed
@paologaleotti paologaleotti deleted the feature/eservice-clientdelegable-PIN-5555 branch November 27, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants