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

Extend authorized parties with instance delegations #856

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

andreasisnes
Copy link
Contributor

Extend authorized parties with instance delegations.

Related Issue(s)

Developer/Reviewer Checklist

  • Your code builds clean without any errors or warnings
  • No changes to config/appsettings or environment variables created in separate linked PR
  • Manual testing done (required)
  • Relevant integration test added (required for functional changes)
  • Relevant automated test added (required for functional changes)
  • All integration tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@andreasisnes andreasisnes marked this pull request as ready for review October 24, 2024 07:14
/// <summary>
/// Composite Key instances
/// </summary>
public class Resource
Copy link
Contributor

Choose a reason for hiding this comment

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

Burde ikke denne hete noe med Instance for ikke å blande den med AuthorizedResources som er av type string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm, skal rename den

Copy link
Contributor Author

@andreasisnes andreasisnes Oct 25, 2024

Choose a reason for hiding this comment

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

blir kanskje ikke så synelig i File Changes PRen. Men klassen er definert inne i klassen AuthorizedParty. Så for å referere til klassen Resource utenfor klassen AuthorizedParty så må man bruke AuthorizedParty.Resource. Uansett, så endret jeg navnet til AuthorizedResource. Så det blir nå AuthorizedParty.AuthorizedResource.

@@ -164,10 +164,18 @@ public async Task<List<AuthorizedParty>> GetAuthorizedPartiesForEnterpriseUserUu
return await Task.FromResult(new List<AuthorizedParty>());
}

private async Task<List<InstanceDelegationChange>> GetInstanceDelegations(int subjectUserId, List<int> subjectPartyIds, CancellationToken cancellationToken)
{
var userId = subjectUserId != 0 ? subjectUserId.SingleToList() : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Det blir vel feil å legge inn en userid i en liste over partyids siden disse ikke brukes likt og ikke har samme verdi en user har en party id men den er ikke lik userid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jaja, var en god del issues med denne logikken. Har skrevet den om nå, ble ganske feil.


if (authorizedPartyDict.TryGetValue(instanceParty.PartyId, out var authorizedParty))
{
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Vil ikke denne continue blokken medføre at bare nye partyid får lagt til nye instanser om du har flere instanser så er det bare den første som blir lagt til og dersom du har en annen tilgang så blir ingen instanser lagt til siden den allerede ligger i dictionarien.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm, har skrevet om logikken her.

WHERE
touuid = ANY(@toUuid)
GROUP BY
resourceid, touuid, fromuuid;
Copy link
Contributor

Choose a reason for hiding this comment

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

Samme som JK dersom du ikke har instanceId i group by så vil den slå sammen alle instansene desuten er det vel ikke mulig å bruke en group by og hente ut alle kolonner for de kolenne som ikke er grupert på vil kunne være mange verdier så de må ha en agregate funksjon for å finne hvilken som skal brukes. Jeg ville brukt Common table expression her og grupere på de dataene som var viktige og så hente ut max(instancedelegationchangeid) og deretter joine med denne common table expressionen med tabellen på nytt med id som match så ville jeg få ut alle siste endringer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stemmer, templating før jeg fikk testet spørringen. Endret på spørringen nu

Copy link

sonarcloud bot commented Oct 25, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
52.8% Coverage on New Code (required ≥ 65%)
50.0% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarCloud

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