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

Feature/197 edit open absences #210

Merged
merged 7 commits into from
Sep 24, 2020
Merged

Conversation

caebr
Copy link
Collaborator

@caebr caebr commented Sep 24, 2020

Auf diesem Branch ist zusätzlich noch #208 und die Anzeige der Designation auf "Offene Absenzen" umgesetzt.

Copy link
Collaborator

@hupf hupf left a comment

Choose a reason for hiding this comment

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

Ein paar verschachtelte subscribes, sieht sonst gut aus.

// Disable confirmation value select when unexcused
confirmationValueControl.valueChanges
.pipe(takeUntil(this.destroy$))
.subscribe(this.updateAbsenceTypeIdDisabled.bind(this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hier hast du auch wieder verschachtelte subscribes. Theoretisch könnten auch mehrere FormGroups emitted werden, dann solltest du z.B. immer auf das neue valueChanges wechseln (und das alte unsubscriben). Zum Glück macht switchMap dies bereits von Haus aus.

Ich habe hier mal eine Alternative skizziert, mit zwei neuen Helpers, die wir im shared/utils/form.ts hinzufügen könnten:

function getControl(
  formGroup$: Observable<FormGroup>,
  controlName: string
): Observable<Option<AbstractControl>> {
  return this.formGroup$.pipe(
    map((formGroup) => {
      const control = formGroup.get(controlName);
      return control || null;
    })
  );
}

function getControlValueChanges<T>(
  formGroup$: Observable<FormGroup>,
  controlName: string
): Observable<T> {
  return getControl(this.formGroup$, controlName).pipe(
    switchMap((control) => (control ? control.valueChanges : empty())) // Mit dem empty()  sollte eigentlich kein Wert emitted werden, sondern das innere Observable einfach completen
  );
}

// Disable confirmation value select when unexcused
getControlValueChanges(this.formGroup$, "confirmationValue")
  .pipe(takeUntil(this.destroy$))
  .subscribe(this.updateAbsenceTypeIdDisabled.bind(this));

// Disable form when saving
combineLatest([
  getControl(this.formGroup$, "confirmationValue"),
  getControl(this.formGroup$, "absenceTypeId"),
  this.saving$,
])
  .pipe(takeUntil(this.destroy$))
  .subscribe(([confirmationValueControl, absenceTypeIdControl, saving]) => {
    if (saving) {
      confirmationValueControl.disable();
      absenceTypeIdControl?.disable();
    } else {
      confirmationValueControl.enable();
      this.updateAbsenceTypeIdDisabled(confirmationValueControl.value);
    }
  });

// Initially select excused state radio button
combineLatest([
  getControl(this.formGroup$, "confirmationValue").pipe(filter(notNull)),
  this.excusedState$,
])
  .pipe(takeUntil(this.destroy$))
  .subscribe(([confirmationValueControl, excusedState]) =>
    confirmationValueControl.setValue(excusedState.Key)
  );

this.save(confirmationValue, absenceTypeId);
});
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Hier wäre ein combineLatest angebracht, mit den beiden Observables, dann ein einzelnes subscribe wo du beide Werte erhälst.
  2. Aber der Wert vom excusedState$ wir hier gar nicht mehr gebraucht, das könntest du also noch entfernen.

: absenceTypeIdControl.disable()
);
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Da wären wir dann beim combineLatest:

combineLatest([
  getControl(this.formGroup$, 'absenceTypeId').pipe(take(1)), // Das wäre der Helper von weiter oben
  this.excusedState$.pipe(take(1)),
]).subscribe(([absenceTypeIdControl, excusedState]) => {
  if (absenceTypeIdControl && excusedState) {
    confirmationValue === excusedState.Key
      ? absenceTypeIdControl.enable()
      : absenceTypeIdControl.disable();
  }
});

@@ -21,6 +21,45 @@ export function getIdsGroupedByPerson(
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wird das getIdsGroupedByPerson noch sonst wo verwendet? Sonst könnten wir es auch löschen...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Es wird noch in MyAbsencesReportSelectionService und EditAbsencesSelectionService verwendet.

@caebr caebr force-pushed the feature/197-edit-open-absences branch from 06ebc71 to 210f1c6 Compare September 24, 2020 13:18
@caebr caebr force-pushed the feature/197-edit-open-absences branch from 210f1c6 to a9e6c71 Compare September 24, 2020 17:09
@caebr caebr merged commit 78d5731 into master Sep 24, 2020
@mfehlmann mfehlmann deleted the feature/197-edit-open-absences branch April 21, 2022 11:42
@mfehlmann mfehlmann restored the feature/197-edit-open-absences branch April 21, 2022 11:42
@mfehlmann mfehlmann deleted the feature/197-edit-open-absences branch April 21, 2022 11:42
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.

None yet

2 participants