Skip to content

Conversation

@Majjki
Copy link
Contributor

@Majjki Majjki commented Nov 24, 2025

Filtrer skjuler avtaler som har opphav ARENA og er mentoravtaler for rolle som ikke er beslutter/veileder

@Majjki Majjki requested a review from Copilot November 25, 2025 11:41
Copilot finished reviewing on behalf of Majjki November 25, 2025 11:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces filtering logic to hide mentor agreements migrated from ARENA for users who are not internal users (veileder/beslutter). The filter is applied both when listing agreements and when checking individual agreement access.

  • Added filtering in hentAvtalerMedLesetilgang to exclude ARENA mentor agreements from list results
  • Added access check in sjekkTilgang to prevent direct access to hidden ARENA mentor agreements
  • Implemented harTilgangTilMentorArenaMigrering method containing the filtering logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (!avtalenEksisterer(avtale)) {
throw new RessursFinnesIkkeException();
}
if (skalSkjulesMentorArenaMigrering(avtale)) {
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The method skalSkjulesMentorArenaMigrering does not exist. This should be harTilgangTilMentorArenaMigrering(avtale) to match the method defined on line 223. Additionally, the logic needs to be inverted since harTilgangTilMentorArenaMigrering returns false when the agreement should be hidden, so the condition should be !harTilgangTilMentorArenaMigrering(avtale).

Suggested change
if (skalSkjulesMentorArenaMigrering(avtale)) {
if (!harTilgangTilMentorArenaMigrering(avtale)) {

Copilot uses AI. Check for mistakes.
@Majjki Majjki requested a review from Copilot November 27, 2025 09:47
Copilot finished reviewing on behalf of Majjki November 27, 2025 09:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
return new Tilgang.Tillat();
}));
return tilgangsmappe;
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The removed avtalenEksisterer override previously hid ALL Arena agreements that weren't concluded (!erAvtaleInngått()) from employers. The new filtering logic in Avtalepart only hides MENTOR Arena agreements based on completion status. This means non-mentor Arena agreements that aren't concluded will now be visible to employers. If this behavior change is intentional, consider documenting it; otherwise, the old logic may need to be preserved or combined with the new filter.

Copilot uses AI. Check for mistakes.

public boolean sjekkOmAlltErFylltUtUntattFamiljeTilknyting() {

if (!felterSomIkkeErFyltUt().stream().filter(x -> !x.equals("harFamilietilknytning")).toList().isEmpty()) {
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The double negation with .filter(x -> !x.equals("harFamilietilknytning")).toList().isEmpty() makes this logic confusing. This checks if the filtered list is empty, which means all unfilled fields are "harFamilietilknytning". Consider simplifying to:

if (felterSomIkkeErFyltUt().stream().anyMatch(x -> !x.equals("harFamilietilknytning")))

This is clearer: "if any unfilled field is NOT harFamilietilknytning, return false".

Suggested change
if (!felterSomIkkeErFyltUt().stream().filter(x -> !x.equals("harFamilietilknytning")).toList().isEmpty()) {
if (felterSomIkkeErFyltUt().stream().anyMatch(x -> !x.equals("harFamilietilknytning"))) {

Copilot uses AI. Check for mistakes.
}
}

public boolean sjekkOmAlltErFylltUtUntattFamiljeTilknyting() {
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Method name has a typo: "Allt" should be "Alt" (singular, consistent with sjekkOmAltErKlarTilGodkjenning on line 788). Additionally, "FamiljeTilknyting" doesn't match the field name "Familietilknytning" used elsewhere, though this inconsistency exists throughout method names in this class.

Copilot uses AI. Check for mistakes.
Comment on lines +829 to +831
public boolean erKlarForVisningAvMigrertMentorAvtale() {
return sjekkOmAlltErFylltUtUntattFamiljeTilknyting();
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

This method is a simple wrapper that adds no additional logic or documentation. Consider removing it and directly calling sjekkOmAlltErFylltUtUntattFamiljeTilknyting() from the caller in Avtalepart.java line 237, or add documentation explaining why this wrapper exists.

Suggested change
public boolean erKlarForVisningAvMigrertMentorAvtale() {
return sjekkOmAlltErFylltUtUntattFamiljeTilknyting();
}
// Method removed: erKlarForVisningAvMigrertMentorAvtale()

Copilot uses AI. Check for mistakes.
Comment on lines +237 to +243
boolean utFylltUntattFamiljeTilknyting = avtale.erKlarForVisningAvMigrertMentorAvtale();

if (!erInternRolle && !utFylltUntattFamiljeTilknyting) {
log.info("Skjult mentoravtale med opphav Arena for migrering. Rolle: {}, AvtaleNr: {}", rolle(), avtale.getAvtaleNr());
}

return erInternRolle || utFylltUntattFamiljeTilknyting;
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Variable name violates camelCase convention: "utFyllt" should be "utfylt" (lowercase 'f'). Additionally, "FamiljeTilknyting" appears to be a misspelling that doesn't match the field name "Familietilknytning", though this inconsistency exists in related method names.

Suggested change
boolean utFylltUntattFamiljeTilknyting = avtale.erKlarForVisningAvMigrertMentorAvtale();
if (!erInternRolle && !utFylltUntattFamiljeTilknyting) {
log.info("Skjult mentoravtale med opphav Arena for migrering. Rolle: {}, AvtaleNr: {}", rolle(), avtale.getAvtaleNr());
}
return erInternRolle || utFylltUntattFamiljeTilknyting;
boolean utfyltUntattFamilietilknytning = avtale.erKlarForVisningAvMigrertMentorAvtale();
if (!erInternRolle && !utfyltUntattFamilietilknytning) {
log.info("Skjult mentoravtale med opphav Arena for migrering. Rolle: {}, AvtaleNr: {}", rolle(), avtale.getAvtaleNr());
}
return erInternRolle || utfyltUntattFamilietilknytning;

Copilot uses AI. Check for mistakes.
Comment on lines +228 to +244
private boolean mentorAvtaleUnderMigreringSkalVises(Avtale avtale) {
boolean erMentorArena = erAvtaleMigrertMentorArena(avtale);

if (!erMentorArena) {
return true;
}

boolean erInternRolle = rolle().erInternBruker();

boolean utFylltUntattFamiljeTilknyting = avtale.erKlarForVisningAvMigrertMentorAvtale();

if (!erInternRolle && !utFylltUntattFamiljeTilknyting) {
log.info("Skjult mentoravtale med opphav Arena for migrering. Rolle: {}, AvtaleNr: {}", rolle(), avtale.getAvtaleNr());
}

return erInternRolle || utFylltUntattFamiljeTilknyting;
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The new filtering logic for migrated mentor agreements from Arena lacks test coverage. Consider adding tests for:

  1. Non-internal users being denied access to incomplete migrated mentor agreements
  2. Internal users (VEILEDER/BESLUTTER) having access to all migrated mentor agreements
  3. Non-internal users having access to completed migrated mentor agreements
  4. Non-mentor or non-Arena agreements not being affected by the filter

Copilot uses AI. Check for mistakes.
Comment on lines +814 to +827
public boolean sjekkOmAlltErFylltUtUntattFamiljeTilknyting() {

if (!felterSomIkkeErFyltUt().stream().filter(x -> !x.equals("harFamilietilknytning")).toList().isEmpty()) {
log.warn(
"Avtale= {}, med type= {} har ikke alle felter fylt ut for visning= {}",
this.avtaleNr,
this.tiltakstype,
felterSomIkkeErFyltUt()
);
return false;
}
log.info("Migrert mentoravtale {} er klar for visning.", this.avtaleNr);
return true;
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The new method sjekkOmAlltErFylltUtUntattFamiljeTilknyting() lacks test coverage. Consider adding tests to verify:

  1. Returns true when all fields are filled except "harFamilietilknytning"
  2. Returns false when any other field besides "harFamilietilknytning" is not filled
  3. Returns true when all fields including "harFamilietilknytning" are filled

Copilot uses AI. Check for mistakes.
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.

2 participants