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

BC - 4655 migration-of-federal-states #4385

Closed
wants to merge 46 commits into from

Conversation

wolfganggreschus
Copy link
Contributor

@wolfganggreschus wolfganggreschus commented Sep 4, 2023

Description

PR will execute the migration from feathers to nestJs regarding FederalState.

Links to Tickets or other pull requests

BC-4655

Changes

FederalStates will be available through "/api/v3/federal-states". The old federal state feathers path is still used.
In a follow up ticket federalStates will no longer be available through the "/federalStates" route, but instead through "/api/v3/federal-states".

This has to be deleted in the follow up:
https://github.com/hpi-schul-cloud/schulcloud-server/tree/main/src/services/federalState
any reference to it from client, must be changed to the new v3 path
any reference to it from server, must somehow load nestjs service instead. Example https://github.com/hpi-schul-cloud/schulcloud-server/blob/main/src/services/sync/strategies/TSP/TSPBaseSyncer.js#L186

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Approval for review

  • DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

apps/server/src/shared/domain/entity/index.ts Outdated Show resolved Hide resolved
apps/server/src/shared/domain/entity/school.entity.ts Outdated Show resolved Hide resolved
createdAt: new Date(2020, 1),
updatedAt: new Date(2020, 1),
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above (DoFactory) - should be unique, or?

tsconfig.json Outdated Show resolved Hide resolved
@Module({
imports: [FederalStateModule],
controllers: [FederalStateController],
providers: [FederalStateUC],
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is ok...

apps/server/src/modules/federal-state/index.ts Outdated Show resolved Hide resolved
@hpi-schul-cloud hpi-schul-cloud deleted a comment from virgilchiriac Sep 12, 2023
return federalStateResponse;
}

@Post()
Copy link
Contributor

Choose a reason for hiding this comment

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

ApiResponse missing

const createdFederalState = await this.federalStateService.create(federalStateCreate);
return createdFederalState;
}

// TODO: names could be an enum
async findFederalStateByName(name: string) {
const federalState = await this.federalStateService.findFederalStateByName(name);
Copy link
Contributor

Choose a reason for hiding this comment

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

no authorization here?

@virgilchiriac
Copy link
Contributor

TODO:
update not working
autorization in uc not working
tests
missing / inclomplete
one test in provisioning oidc module uses federal state for some reason and is not working
modules\provisioning\strategy\oidc\service\oidc-provisioning.service.ts : 50
enum federal state name should be moved?

entity <-> county
the _id missing. Does it need it?

pagination - why is that necesary?

findFederalStateByName - how was that called? was in feathers, but how is used

@wolfganggreschus wolfganggreschus deleted the BC-4655-migration-of-federal-states branch June 5, 2024 09:23
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