Oke #314
Oke #314
Conversation
There was a problem hiding this comment.
@hamrt thanks for all the hard work!
It is a big PR to review, so we might have to do this in changes. Or merge quite soon to a v6.0-dev folder as discussed and continue finetuning there.
This review is my first pass on all the changes. I might have to do another after we've talked some more about some of the design choices which I don't always understand fully.
Let's meet soon to discuss my review and figure out what to change and what to keep as-is.
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
Deze file hoort wat mij betreft niet thuis in de spec. Je zou wel de .vscode folder aan .gitigignore kunnen toevoegen?
CHANGELOG.md
Outdated
There was a problem hiding this comment.
Ik denk dat we, naast de changelog, ook nog een wat verhalender tekstje moeten toevoegen wat samenvat wat al deze changes "verbind".
| - denied | ||
| - associated | ||
| - queued | ||
| - finished |
There was a problem hiding this comment.
Adding finished to the possible states of an association seems to conflate two things:
- The state of the association
- The state of the offering
The student remains associated with the offering, even when the offering is finished. I would suggest that this enumeration not be changed and to introduce a different mechanism for tracking the finished/unfinished state.
Some more explanation about what exactly finished means would also be welcome.
There was a problem hiding this comment.
The offering itself is not finished the student is finished with his or her exam, meaning the assocaition has reached a ststus where a resutl can be expected, or where a situation where nog result but a not present or other situation has occurred, indicating a change in the status of the association it self. This additional information could be provided
v5.1/enumerations/modeOfStudy.yaml
Outdated
| - part-time | ||
| - dual training | ||
| - self-paced | ||
| - extraneous |
There was a problem hiding this comment.
The modeOfStudy attribute describes the education, not the way of participation by a student. Why was this added here?
There was a problem hiding this comment.
NO it describes the type of study, the person can only enroll for an exam in this case there are no other modes of study available for the student
| - denied | ||
| - associated | ||
| - queued | ||
| - finished |
There was a problem hiding this comment.
Also see my comment for associationState
v5.1/paths/OfferingInstance.yaml
Outdated
| summary: PUT /offerings/{offeringId} | ||
| description: | | ||
| PUT / create an offering or update a single offering based on the offeringID provided. | ||
| You want to PUT a resource to the same URI you intend to GET it from. also check RFC 72314.3.4 PUT. |
There was a problem hiding this comment.
RFC7231 is obsoleted by RFC9110.
There was a problem hiding this comment.
I don' really understand yet how all these ComponentOfferingAssoications***.yaml files work together and why all are needed. Let's look at that together soon.
| description: The first day this EducationSpecification is valid (inclusive). | ||
| type: string | ||
| format: date | ||
| example: 2024-12-15T08:50:00+01:00 |
There was a problem hiding this comment.
See other comments about date-time
There was a problem hiding this comment.
that is a valid remark, I am curious though how this is implemented. Maybe we should also check how the spec is implemented.
| description: The day this EducationSpecification ceases to be valid (e.g. exclusive). | ||
| type: string | ||
| format: date | ||
| example: 2025-12-15T08:50:00+01:00 |
There was a problem hiding this comment.
See other comments about date-time
v5.1/spec.yaml
Outdated
| $ref: paths/DocumentInstance.yaml | ||
| /associations/{associationId}: | ||
| $ref: paths/AssociationInstance.yaml | ||
| /associations/{associationId}/url: |
There was a problem hiding this comment.
We don't need this one
All updates made to the specification based on the requirements for the OKE project.
All changes are documented in the changelog.md