new version of handling besluit information #45

Rahien · 2024-09-13T14:17:21Z

Description

Besluiten are now handled as described on the gitbook: https://app.gitbook.com/o/-MP9Yduzf5xu7wIebqPG/s/UYHyp4MV7ApSjjQn2DUx/algemene-informatie/besluit-harvesting

How to test

Use the mock route to receive besluit instances in the consumer graph. Verify that indeed they are processed as per the gitbook.

After each mock route check, run the clear decisions endpoint: localhost:8082/mock/clear-decisions

The mock routes are configured for use in provincie vlaams brabant as an example bestuurseenheid. Because of a delay of delta message processing you need to wait 10s before you see the notifications.

mock routes exist for:

http://localhost:8082/mock/add-decision: default case, nothing special. Changes status to bekrachtigd and adds an info notification
http://localhost:8082/mock/add-decision?new=true: unknown mandataris, created on our end with notification
http://localhost:8082/mock/add-decision?fraction=true: unknown fraction, error message
http://localhost:8082/mock/add-decision?unknownMandaat=true: unknown mandaat, error message
http://localhost:8082/mock/add-decision?incompletePerson=true: incomplete person, error message
http://localhost:8082/mock/add-decision?changeFractionName=true: different fraction name
http://localhost:8082/mock/add-decision?newBeleidsdomein=true: new beleidsdomein, info message

Links to other PR's

ignore the besluiten graph in most deltas. add type and links to notifications app-lokaal-mandatenbeheer#212

JonasVanHoof

Just went over the code, mostly some things about variables or doing the same in a different way.

JonasVanHoof · 2024-11-08T15:51:04Z

data-access/lidmaatschap.ts

+  graph: string,
+) {
+  const id = uuidv4();
+  const intervalUri = `http://data.lblod.info/id/tijdsintervallen//${id}`;


Does it need to be //?

JonasVanHoof · 2024-11-08T15:54:31Z

data-access/mandataris.ts

  if (sparqlresult?.artikel) {
-    return sparqlresult.artikel;
+    return {
+      besluit: sparqlresult.artikel.value,
+      link: sparqlresult.link.value,
+    };
  }

  return null;
 }



Do you prefer that the returning value of this function is of a different format?

Would remove the if and just add a ? after sparqlResult.artikel?.value. Than you do have to change the logic where this method is called to check if (findDecisionForMandataris(URI).besluit) instead of just the return value

JonasVanHoof · 2024-11-08T15:55:34Z

data-access/mandataris.ts

  }

  return null;
 }

 export async function updatePublicationStatusOfMandataris(
-  mandataris: Term,
+  mandataris: string,


Should we create a URI type so we work with these and errors can be thrown when it is not a URI?

afaik a uri is just a string

JonasVanHoof · 2024-11-08T15:56:10Z

data-access/mandataris.ts

+    mandataris: sparqlEscapeUri(mandataris),
    status: sparqlEscapeUri(status),
    mandatarisType: sparqlEscapeTermValue(TERM_MANDATARIS_TYPE),


If we change mandataris to escapeUri we should also do it for the type

there is a separate story to remove all uses of term from the mandataris service.

JonasVanHoof · 2024-11-08T15:58:42Z

routes/delta-decisions.ts

+  // keep these mandataris instances in the database instead of memory
+  // so if we crash we know they should still be processed
+  // wait BUFFER_TIME to process the mandataris so we are reasonably sure that we have all the  info


JonasVanHoof · 2024-11-08T16:26:28Z

controllers/mandatees-decisions.ts

-  );
-  console.log(
-    `|> Found ${incomingTriples.length} triples in the staging graph for mandataris.`,
+async function isValidMandataris(mandataris: string) {


*mandatarisUri

JonasVanHoof · 2024-11-08T16:27:55Z

controllers/mandatees-decisions.ts

+        ? ('ontslag' as const)
+        : ('aanstelling' as const),


Or do we use an enum for this?

JonasVanHoof · 2024-11-08T16:29:49Z

cron/handle-decision-queue.ts

+    } LIMIT ${BESLUIT_BATCH_SIZE}`;
+
+  const result = await querySudo(query);
+  return result.results.bindings.map((binding) => {


getSparqlResults.map(... *

JonasVanHoof · 2024-11-08T16:31:07Z

data-access/lidmaatschap.ts

+  const safeMandatarisUri = sparqlEscapeUri(mandatarisUri);
+  const safeGraph = sparqlEscapeUri(graph);
+  const safeIntervalUri = sparqlEscapeUri(intervalUri);


uniform: do we want to do separate escapes or use the const escaped = { ... } as did before?

JonasVanHoof · 2024-11-08T16:31:35Z

data-access/mandataris.ts

-  mandataris: Term,
-): Promise<Term | null> {
-  const mandatarisSubject = sparqlEscapeTermValue(mandataris);
+  mandataris: string,


mandatarisUri*

JonasVanHoof

Tried out the mock routes

Karel's: besluiten processing
- First mock route looks to be working correctly ✅
- second ✅
- third a notification is created for the unknown fractie ✅
- fourth: unknown mandaat error => checkIfMinimalMandatarisInfoAvailable returned and not throwing an error.. ❌
- incomplete person, error message => checkIfMinimalMandatarisInfoAvailable returned and not throwing an error.. ❌
- different fraction name => checkIfMinimalMandatarisInfoAvailable returned and not throwing an error.. ❌
- new beleidsdomein, info message => checkIfMinimalMandatarisInfoAvailable returned and not throwing an error.. ❌
It seems that the minimal info is often not valid because of the graph and after that the publication status is still changed to bekrachtigt?

Im open to check these last mock calls togheter!

JonasVanHoof · 2024-11-12T08:54:12Z

routes/delta-decisions.ts

+      (quad) =>
+        quad.graph.value.startsWith(BESLUIT_STAGING_GRAPH) &&
+        quad.predicate.value.startsWith(
+          'http://data.vlaanderen.be/ns/mandaat#bekrachtigt',


In the docs it just says to process mandaat:bekrachtigtAanstellingVan (first point). A mandataris should only be set to bekrachtigd when it is an aanstelling?

I guess it makes sense to handle both, even if it not mentioned in the docs, but I think the outcome should be different, but now they seem to be handled the exact same way?

JonasVanHoof · 2024-11-12T09:00:48Z

cron/handle-decision-queue.ts

+  for (const match of batch) {
+    await safeProcessMandatarisForDecisions(match);
+  }
+  await cleanInstancesFromQueue(batch);


Do we also need to remove the batch from the staging graph?

… mock route

bfidlers

The besluiten logic seems pretty solid, few remarks here and there. The code is not always as readable, a few more descriptive function names would help.

Few issues however with older logic, a method disappeared which is still used and a method seems to mix ids and uris, but this might just be the name that is misleading.

bfidlers · 2024-11-05T13:37:25Z

app.ts

@@ -49,3 +50,4 @@ const errorHandler: ErrorRequestHandler = function (err, _req, res, _next) {
 app.use(errorHandler);

 notificationBekrachtigdMandataris.start();
+setTimeout(() => cronjob.start(), 10000);


We should probably remove this when actually merging this I guess?

bfidlers · 2024-11-05T13:40:51Z

config/custom-dispatching/delta-context-config.js

Bit confused about these http vs https prefixes, both seem to be in use in our application.

bfidlers · 2024-11-05T13:44:08Z

controllers/mandataris.ts

+async function findDecision(mandatarisUri: string): Promise<string | null> {
+  const isMandataris = await mandataris.isValidId(mandatarisUri);
  if (!isMandataris) {
    throw new HttpError(
-      `Mandataris with id ${mandatarisId} not found.`,
+      `Mandataris with id ${mandatarisUri} not found.`,
      STATUS_CODE.BAD_REQUEST,
    );
  }
-  const decision = await findDecisionForMandataris({
-    type: 'uri',
-    value: mandatarisId,
-  } as Term);
+  const decision = await findDecisionForMandataris(mandatarisUri);


Bit confused about this, what is it? a uri or an id, because some statements seem to imply the first, other the second.

bfidlers · 2024-11-15T08:15:58Z

config/custom-dispatching/delta-context-config.js

@@ -11,15 +11,15 @@ PREFIX euro: <http://data.europa.eu/m8g/>
 PREFIX euvoc: <http://publications.europa.eu/ontology/euvoc#>
 PREFIX ext: <http://mu.semte.ch/vocabularies/ext/>
 PREFIX foaf: <http://xmlns.com/foaf/0.1/>
-PREFIX generiek: <https://data.vlaanderen.be/ns/generiek#>
+PREFIX generiek: <http://data.vlaanderen.be/ns/generiek#>


I'm not sure about this generiek prefix, if I look in the app, sometimes we seem to use http and sometimes https.

bfidlers · 2024-11-15T08:25:34Z

util/uuid-for-uri.ts

+export function getIdentifierFromPersonUri(uri: string) {
+  const personBaseUri = 'http://data.lblod.info/id/personen/';
+  if (!uri.includes(personBaseUri)) {
+    return uuidv4();
+  }
+
+  return uri.replace(personBaseUri, '').trim();
+}


This is not used anymore.

bfidlers · 2024-11-18T09:01:31Z

controllers/mandatees-decisions.ts

-  );
-  console.log(
-    `|> Found ${incomingTriples.length} triples in the staging graph for mandataris.`,
+async function isValidMandataris(mandataris: string) {


Function name is not really descriptive. It does more than it says. It checks whether the madataris is valid and whether it has a besluit. Maybe you could call it isValidMandatarisWithBesluit ?

bfidlers · 2024-11-18T09:05:43Z

data-access/mandatees-decisions.ts

+
+export const BESLUIT_STAGING_GRAPH =
+  process.env.BESLUIT_STAGING_GRAPH ||
+  'http://mu.semte.ch/graphs/besluiten-consumed';

 export const TERM_STAGING_GRAPH = {


This isn't used anymore.

bfidlers · 2024-11-18T09:08:41Z

data-access/mandatees-decisions.ts

+      GRAPH ?bestuurseenheidGraph {
+        ?mandaat a mandaat:Mandaat .
+      }
+      ?bestuurseenheidGraph ext:ownedBy ?bestuurseenheid.


Is this used so you don't have to exclude all special graphs?

bfidlers · 2024-11-18T09:09:59Z

data-access/mandatees-decisions.ts

+    PREFIX ext: <http://mu.semte.ch/vocabularies/ext/>
+    PREFIX person: <http://www.w3.org/ns/person#>
+
+    SELECT * {


The * seems a bit inefficient if you only need the bestuurseenheidGraph.

bfidlers · 2024-11-18T09:12:36Z

data-access/mandatees-decisions.ts

+      OPTIONAL {
+        ?graph ext:ownedBy ?bestuurseenheid.
+      }
+      FILTER(?graph = <http://mu.semte.ch/graphs/public> || BOUND(?bestuurseenheid))


I think public now also has the ext:ownedBy?

bfidlers · 2024-11-18T09:40:44Z

We need to do a second pass over the code to check we always exclude the besluiten staging graph. Now it is often implicit and sometimes the ext:ownedBy is used. But we need to double check this and make it more explicit.

bfidlers · 2024-11-18T10:16:36Z

Testing:

http://localhost:8082/mock/add-decision: ✅
http://localhost:8082/mock/add-decision?new=true: ✅
http://localhost:8082/mock/add-decision?fraction=true:❓ does throw an error, but the bekrachtiging is made
http://localhost:8082/mock/add-decision?unknownMandaat=true: ✅
http://localhost:8082/mock/add-decision?incompletePerson=true: ❓ it gives the missing minimal information message, but should it create a new person with incomplete information? -> in the checkIfMinimalMandatarisInfoAvailable we require foaf:familyName and persoon:gebruikteVoornaam, but the incomplete person only has foaf:name
http://localhost:8082/mock/add-decision?changeFractionName=true: ❓it works, but I think the change fractie name should also get a warning message
http://localhost:8082/mock/add-decision?newBeleidsdomein=true: ✅ works, but the old beleidsdomeinen not on the bekrachtigde mandataris are removed.

bfidlers · 2024-11-18T10:38:59Z

Noticed two more things.

We always update the publication status, is this correct? For example if we throw an error because the mandate in the besluit is unknown, do we still want to bekrachtig the mandataris with that uri?
I feel like we never actually create the link to the besluit?

Rahien force-pushed the karel/lmb-758-rework-besluit-info branch from 593dfca to 993f177 Compare September 13, 2024 14:30

Rahien mentioned this pull request Sep 13, 2024

ignore the besluiten graph in most deltas. add type and links to notifications lblod/app-lokaal-mandatenbeheer#212

Merged

Rahien marked this pull request as ready for review September 13, 2024 14:38

Rahien mentioned this pull request Oct 4, 2024

add links to mandataris instances lblod/frontend-lokaal-mandatenbeheer#339

Merged

This was referenced Oct 15, 2024

LMB-666 | Validate besluiten flow through mock endpoint #43

Closed

Lmb 666/validate besluit lmb flow #42

Closed

Rahien force-pushed the karel/lmb-758-rework-besluit-info branch from 6d2472c to a233d4b Compare October 25, 2024 11:00

Rahien force-pushed the karel/lmb-758-rework-besluit-info branch 2 times, most recently from d6638e7 to 873ac7a Compare November 6, 2024 07:43

JonasVanHoof reviewed Nov 8, 2024

View reviewed changes

JonasVanHoof reviewed Nov 12, 2024

View reviewed changes

karel kremer added 7 commits November 15, 2024 16:39

initial version of new besluit handling, tested with the cases in the…

8f3d1d8

… mock route

remove old way of handling decisions

3efdc16

linting

54540a6

less techy notifications

2a180cd

forgot to add a uuid to the link

c973638

typo

6f1fe42

fix linting

f93e979

Rahien force-pushed the karel/lmb-758-rework-besluit-info branch from 873ac7a to f93e979 Compare November 15, 2024 15:39

bfidlers requested changes Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new version of handling besluit information #45

new version of handling besluit information #45

Rahien commented Sep 13, 2024 •

edited

Loading

JonasVanHoof left a comment

JonasVanHoof Nov 8, 2024

JonasVanHoof Nov 8, 2024

JonasVanHoof Nov 8, 2024

Rahien Nov 15, 2024

JonasVanHoof Nov 8, 2024

Rahien Nov 15, 2024

JonasVanHoof Nov 8, 2024

JonasVanHoof Nov 8, 2024

JonasVanHoof Nov 8, 2024

JonasVanHoof Nov 8, 2024

JonasVanHoof Nov 8, 2024

JonasVanHoof Nov 8, 2024

JonasVanHoof left a comment

JonasVanHoof Nov 12, 2024

bfidlers Nov 18, 2024

JonasVanHoof Nov 12, 2024

bfidlers left a comment

bfidlers Nov 5, 2024

bfidlers Nov 5, 2024

bfidlers Nov 5, 2024

bfidlers Nov 15, 2024

bfidlers Nov 15, 2024

bfidlers Nov 18, 2024

bfidlers Nov 18, 2024

bfidlers Nov 18, 2024

bfidlers Nov 18, 2024

bfidlers Nov 18, 2024

bfidlers commented Nov 18, 2024

bfidlers commented Nov 18, 2024

bfidlers commented Nov 18, 2024 •

edited

Loading

new version of handling besluit information #45

Are you sure you want to change the base?

new version of handling besluit information #45

Conversation

Rahien commented Sep 13, 2024 • edited Loading

Description

How to test

Links to other PR's

JonasVanHoof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasVanHoof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfidlers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfidlers commented Nov 18, 2024

bfidlers commented Nov 18, 2024

bfidlers commented Nov 18, 2024 • edited Loading

Rahien commented Sep 13, 2024 •

edited

Loading

bfidlers commented Nov 18, 2024 •

edited

Loading