cdisc-org / ddf-core-poc

This repository will contain the results from the Proof of Concept project.
MIT License
0 stars 1 forks source link

Create rule DDF00039: For a ScheduledInsance that references a ScheduleTimeline, the id of the referenced ScheduledTimeline cannot be the same as the id of the parent #161

Closed BSnoeijerCD closed 5 months ago

BSnoeijerCD commented 7 months ago

Create rule defined for #148

ASL-rmarshall commented 7 months ago

@BSnoeijerCD DDF00039 doesn't seem to be checking the right thing - as currently defined, it is checking that a ScheduledActivityInstance or ScheduledDecisionInstance does not refer to itself in defaultConditionId, which is a duplication of DDF00021. According to the description above, I think this rule should report where timelineId equal_to parent_id. The Description and Message will also need to be corrected.

BSnoeijerCD commented 6 months ago

@ASL-rmarshall : corrected - Thank you for the notice, I went a bit too fast. I think the description was OK. I changed the error message to reflect the correct attributes.

ASL-rmarshall commented 6 months ago
@BSnoeijerCD I've re-checked and the rule logic and message now look good. The only thing that I'd recommend - now that we know that the USDM data conversion includes it - is to include the rel_type column in the test data. As an example, I've added rel_type to the test data for DDF00040 and DDF00041, using the following column headings: rel_type
Type of Relationship
String
[1]

Also, when the entity/class being checked by the rule has instances that can be referenced by id, I'd also recommend including a restriction in the Check to only report where rel_type equal_to "definition". This is so that the same error is not reported multiple times if the problematic instance is referenced by id.

BSnoeijerCD commented 6 months ago

@ASL-rmarshall : I believe that in case it might be confusing to show that, because we are looking at the timeline relationship while the parent is linked via the instances relationship.

ASL-rmarshall commented 6 months ago
@BSnoeijerCD As discussed, the USDM conversion process uses referenced id values to retrieve and represent details of the class/entity instance that is identified by the id value. For this particular rule, this will cause over-reporting of issues if a ScheduledActivityInstance or ScheduledDecisionInstance both references its parent ScheduleTimeline in timelineId and is also specified in the entryId attribute of its parent ScheduleTimeline. In this case, the ScheduledActivityInstance or ScheduledDecisionInstance table will contain a record for both the original definition and the entryId reference. For example: parent_entity parent_id parent_rel rel_type id instanceType timelineId ...
ScheduleTimeline ScheduleTimeline_1 instances definition ScheduledActivityInstance_1 ACTIVITY ScheduleTimeline_1 ...
ScheduleTimeline ScheduleTimeline_1 entryId reference ScheduledActivityInstance_1 ACTIVITY ScheduleTimeline_1 ...

The restriction would be needed to prevent reporting both the "definition" record and (any) "reference" records.

BSnoeijerCD commented 6 months ago

@ASL-rmarshall : I changed the code and test data to include the rel_type. It can be reviewed again.

DianeWold commented 6 months ago

Negative data included 2 ScheduledAcitivityInstance records with timeline reference ids equal to the ids of the parent timeline id and 2 ScheduledDecisionInstance records with timeline reference ids equal to the parent timeline id, resulting in 4 negative results. Positive data had no records with timeline ids that matched the parent. It also included a ScheduledActivityInstance record with no timeline reference and a ScheduledDecisionInsance with no timeline reference, which meant the positve test data returned two "Absent-Variable Skips". This is fine, since a timeline does not need to reference another timeline.