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

Fix base permissionConfig to match Pro and ePA variants (by removing it) #311

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nico-famedly
Copy link

@nico-famedly nico-famedly commented Mar 18, 2025

Pull Request Details

Provide details about your pull request and what it adds, fixes, or changes.

In #259 domainExceptions was renamed to serverExceptions, but only in 2 of the 3 schemas. To avoid confusion update the third schema to match the others by removing it. It isn't referenced by any active specifications anymore.

Breaking Changes

Describe what features are broken by this pull request and why, if any.

<Content>

Issues Fixed

Enter the issue numbers resolved by this pull request below, if any.

<Content>

Other Relevant Information

Provide any other important details below.

This is potentially a breaking change? I don't know why there are 3 schemas and there might be an additional process to follow here, but I think the linked PR should help with that?

@Johennes Johennes self-requested a review March 19, 2025 08:09
Copy link
Contributor

@Johennes Johennes left a comment

Choose a reason for hiding this comment

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

The situation is slightly confusing. The permissionConfig.json was not changed because it was used in gemProdT_TI-M_Client_ePA_PTV_1.0.0-0_V1.0.0 (via TI-M Basis 1.0.0). This release has since been discontinued, however. So I think we can just remove this file entirely on develop.

@nico-famedly
Copy link
Author

I can do that. It is just confusing since TI-M Basis also references invite permissions and now there would be no schema for that. But it doesn't really matter to me either way :)

@nico-famedly nico-famedly force-pushed the nico/fix-server-domain-mix branch from 1f7695f to 6ff3b93 Compare March 19, 2025 13:00
In gematik#259
domainExceptions was renamed to serverExceptions, but only in 2 of the 3
schemas. To avoid confusion remove the third schema, that isn't used in
any active specifications anymore.

Signed-off-by: Nicolas Werner <[email protected]>
@nico-famedly nico-famedly force-pushed the nico/fix-server-domain-mix branch from 6ff3b93 to 804d992 Compare March 19, 2025 13:00
@nico-famedly nico-famedly changed the title Fix base permissionConfig to match Pro and ePA variants Fix base permissionConfig to match Pro and ePA variants (by removing it) Mar 19, 2025
@jason-famedly
Copy link

@Johennes
Copy link
Contributor

I can do that. It is just confusing since TI-M Basis also references invite permissions and now there would be no schema for that.

Yes, like I said, it's very confusing. 🙈

The specs themselves don't give versions for the documents they reference. So TI-M Basis 1.0.0 references permissionConfig.json from "https://github.com/gematik/api-ti-messenger". The version is only specified in the "Produkttypsteckbrief" that uses specs to define a concrete product type.

The only "Produkttypsteckbrief" that uses TI-M Basis 1.0.0 is gemProdT_TI-M_Client_ePA_PTV_1.0.0-0_V1.0.0 and this links to permissionConfig.json at 9b9f21b.

So it is possible to get to the old permissionConfig.json from there. It's not great but I think this is better than keeping the file around on develop / main indefinitely.

@Johennes
Copy link
Contributor

Is this file important in this context? https://github.com/gematik/api-ti-messenger/blob/main/src/schema/examples/permissionConfig.domain-exception.json

Err, yes I guess we can kill the examples, too, then. Good call.

We're also about to kill develop entirely with #313. So the base branch of this PR would need to change. @nico-famedly if this is getting too tedious, feel free to abandon it and I can just nuke the files myself on main after the merge.

@nico-famedly
Copy link
Author

Well, it is not getting to tedious, but it will probably be easier for you to change directly than me trying to catch up. So I will just close it and you fix it directly? :)

@Johennes
Copy link
Contributor

Ok, yeah let me just handle it directly. I'll keep the PR open until then as a reminder. Thanks for surfacing the issue in any case! 🙏

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.

3 participants