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

EW-1006: Add school part to TSP sync. #5279

Merged
merged 13 commits into from
Oct 14, 2024
Merged

EW-1006: Add school part to TSP sync. #5279

merged 13 commits into from
Oct 14, 2024

Conversation

mkreuzkam-cap
Copy link
Contributor

@mkreuzkam-cap mkreuzkam-cap commented Oct 8, 2024

Description

Adds the sync of schools in the new NestJS TSP Sync.

Links to Tickets or other pull requests

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.

@mkreuzkam-cap mkreuzkam-cap changed the title Add school part to TSP sync. EW-1006: Add school part to TSP sync. Oct 10, 2024
@mkreuzkam-cap mkreuzkam-cap marked this pull request as ready for review October 10, 2024 07:51
@Fshmit Fshmit requested review from Fshmit and alweber-cap October 10, 2024 11:58
imports: [
LoggerModule,
ConsoleWriterModule,
...((Configuration.get('FEATURE_TSP_SYNC_ENABLED') as boolean)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not very intuitive or expandable easily. But we have no other use for this currently, so it is ok


const createdSchool = await this.tspSyncService.createSchool(
system,
tspSchool.schuleNummer ?? '',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue that without schuleNummer the data is wrong and should be skipped and logged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tokenEndpoint: system.oauthConfig?.tokenEndpoint ?? '',
});

const lastChangeDate = this.formatChangeDate(new Date(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Set this to a Date 25 hours in the past. Alternatively use an ENV_VAR on how far to go back in time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

public async updateSchool(school: School, name: string): Promise<School> {
school.name = name;
Copy link
Contributor

Choose a reason for hiding this comment

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

this will replace the name of the existing school with an empty name if there is no tspSchool.schuleName provided, should it do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is okay if that's the date sent from the TSP. Schulnummer like Alex pointed out would be more problematic

Copy link
Contributor

Choose a reason for hiding this comment

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

I clarify this with PO

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is empty, please keep the old name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

export class TspSchulnummerMissingLoggable implements Loggable {
getLogMessage(): LogMessage {
const message: LogMessage = {
message: `A TSP school is missing a Schulnummer. Skipping school.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
message: `A TSP school is missing a Schulnummer. Skipping school.`,
message: `A TSP school is missing a Schulnummer. This school is skipped`,

systemService.find.mockResolvedValueOnce([]);
};

it('should throw', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should throw', async () => {
it('should throw a TspSystemNotFound exception', async () => {

export class TspSchulnummerMissingLoggable implements Loggable {
getLogMessage(): LogMessage {
const message: LogMessage = {
message: `A TSP school is missing a Schulnummer. This school is skipped.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

After talking to PO, we can log the Schoolname here to help with support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

@mkreuzkam-cap mkreuzkam-cap merged commit f11a3fa into main Oct 14, 2024
76 checks passed
@mkreuzkam-cap mkreuzkam-cap deleted the EW-1006 branch October 14, 2024 13:46
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