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

Invalid requirement LMS manual test procedure #34

Open
cawerkenthin opened this issue Apr 12, 2022 · 12 comments
Open

Invalid requirement LMS manual test procedure #34

cawerkenthin opened this issue Apr 12, 2022 · 12 comments

Comments

@cawerkenthin
Copy link

cawerkenthin commented Apr 12, 2022

In 004-1-moveOn-Completed.zip, the 7th item indicates a requirement for a block Id to be included in the grouping for a Satisified statement. I can not find this requirement in the cmi5 spec.

@brianjmiller
Copy link
Collaborator

Some context (and hopefully just facts)...

https://github.com/adlnet/CATAPULT/blob/main/lts/__tests__/runtime.js#L267 is what Art is referring to, which includes 9.6.2.3-2 as the requirement identifier, which matches up to https://github.com/adlnet/CATAPULT/blob/main/requirements/requirements.json#L635-L637 which captures the requirement as:

The LMS MUST include the publisher id Activity in the "grouping" context activities list for all "cmi5 defined" and "cmi5 allowed" statements it makes directly in the LRS.

Which does match up to the specification in the named section, 9.6.2.3, https://github.com/AICC/CMI-5_Spec_Current/blob/quartz/cmi5_spec.md#9623-publisher-id-grouping-activity

And now some interpretation (IOW opinions)...

I believe my interpretation of the intent of the section about the publisher id and the use of it in grouping is intended to convey that the object of any cmi5 defined statement will always have the LMS identified Activity but that to make the spec functional in the global sense the corresponding publisher identified Activity should always be included in the statement somewhere and in particular in the grouping context list.

Reading the requirement directly and in isolation, to me, suggests this to be the case. Also, if it is not included then a Satisfied statement in isolation cannot be used to know which globally unique Activity that statement is about, IOW you have to have the mapping of LMS identifier to global identifier somewhere outside of the statement to make sense of it. I consider this very bad.

Having said that... I won't dispute Art's reading of the rest of the text around this specific requirement because it very much does make it seem this is about AUs.

To me clarifying the section to indicate it is not just about AUs is within the limit of how we generally make changes because the requirement as a requirement standalone gives us the room to do so because it does indicate 'all "cmi5 defined" and "cmi5 allowed" statements it [the LMS] makes' which includes Satisfied.

I didn't take the time to look back through the history to see if the movement of the information from 9.6.3.5 publisherId might have led to this confusion.

@thomasturrell
Copy link

OW you have to have the mapping of LMS identifier to global identifier somewhere outside of the statement to make sense of it. I consider this very bad.

I completely agree. This should be a guiding principle.

@thomasturrell
Copy link

Issue 633 might be relevant to this discussion.

@cawerkenthin
Copy link
Author

9.6.2.3 very specifically refers to AU, not blocks. Either:

  1. The spec must change to indicate that blocks also require the publisher ID in the group (+1), or
  2. CATAPULT must change to not enforce this requirement for blocks.

@MrBillMcDonald
Copy link

This issue was discussed at length at the cmi5 working group meeting today (4/29/2022). The current thinking is that section 9.6.2.3 needs clarification and that the Satisfied Statement is a "roll-up" and not "about the AU".

Here are notes from this discussion:

  1. A “publisher id” is specific to an AU. A publisher being an AU author. Blocks are combinations of AU’s and have ID’s but these ID’s. There is no “publisher id” for a block. A Block could conceivably be a mash up of AUs from different publishers.

  2. This forces us into choosing either
    a) an arbitrary or calculated publisher id of an AU within the Block or
    b) all of the AUs within a Block

  3. Similarly, this forces the choice up to each block in the case there are nested block as well to the course

  4. This means each course would have to list a single AU publisher id (dangerous) or all of the publisher ids (exhaustive)

  5. It appears that 9.6.2.3 was intended to be written for all statements that the LMS makes about the AU. A Satisfied statement is usually about BLOCKS. Which is where the Issue was raised.

  6. What we need to understand is what insight are we trying to gain from what we are listing in that (from a single course or block) satisfied Statement. Is it "This is the AU that had activity that ultimately satisfied the Statement?" If so, this is only the last AU, not the only.

@brianjmiller
Copy link
Collaborator

I think this assertion:

A “publisher id” is specific to an AU. A publisher being an AU author.

Is basically false. The AU author is the AU author, the publisher is the publisher of the course. The publisher does have to specify an id for both Blocks and Courses (see id in 13.1.1 and 13.1.2). An AU author may be a publisher, and a publisher may take the advice of the AU author of how to construct a course but any combination of the roles doesn't preclude them from having to set an identifier for all blocks and the course in the package. The identifier in the package at both the block and course level is the publisher id, those things also then get an LMS identifier.

A block being a mashup of AUs from different AU authors is irrelevant, the combination of the set of AUs in a block is the act of publishing and the identifier is established at that time.

It appears that 9.6.2.3 was intended to be written for all statements that the LMS makes about the AU. A Satisfied statement is usually about BLOCKS. Which is where the Issue was raised.

I fundamentally disagree with this statement and some of the history of how things moved within the specification bears this out. I think a review of the history of the crafting of publisher id will be fairly explicit about it being specifically about the notion of a course and block identifier within the course package.

@andyjohnson
Copy link
Contributor

I am totally for the idea of expanding (if the spec were taken literally) the publisher ids to Blocks and Courses as @brianjmiller suggests. I do not want to get into the situations represented by 2-5 above. What I thought was happening is that because they were NOT considered publisher ids is that we were force-fitting AU publisher ids into what should have been Block and Course publisher ids.

@brianjmiller
Copy link
Collaborator

I don't really think it is an expansion. The concept of a publisher id was always intended to pertain to the course and the blocks. The concept was born out of the issue of LMSs having the ability to alter course structure details locally. AICC/CMI-5_Spec_Current#378 is the original issue, and AICC/CMI-5_Spec_Current#431 with AICC/CMI-5_Spec_Current#433 is pertinent for when it became a context activity. 2-5 were effectively already discussed in the original issue and dismissed since the id was already available on a block and course, so it naturally just got recognized as a publisher id. That notion is born out of the LMS id as object is not enough to allow comparing statements across LMSs for the same course structure where same course structure is where the (global) publisher id matches. If you don't do it this way there is no way to compare courses/blocks across LMSs just from statement streams.

@brianjmiller
Copy link
Collaborator

Note requirements from 9.4

The LMS MUST generate a unique ID for each block object. The generated block id MUST NOT match the publisher’s ID from the course structure.

The LMS MUST generate a unique ID for each course object. The generated course id MUST NOT match the publisher’s ID from the course structure.

I think this makes it pretty clear that the identifier in the course structure is a publisher id.

@andyjohnson
Copy link
Contributor

Hey Brian, I agree with it not being "expanding", which is why I had the caveat in there. I agree with the evidence you are presenting that with all intent, it seemed like it should be that AU, Block, and Course should all have publisher ids.

@brianjmiller
Copy link
Collaborator

Some more history reading that came out of the 05/06/2022 call.

I think this is where the confusion came in... AICC/CMI-5_Spec_Current@e3a98d8 this commit was intended to clear some of this language up, but you'll notice that the second change was to include "Block or Course" but did so to the same sentence that was altered in the next to last change which happened under "publisherid" when it was still a context extension. We then in a later commit (AICC/CMI-5_Spec_Current@6232ec5) removed that section thinking it was duplicative but it hadn't been completely duplicated. (IOW the "Block or Course" language dropped out accidentally.)

@MrBillMcDonald
Copy link

Per May 6th Meeting the following pull request was created to address this issue:

See Per May 6th, 2022 meeting - Publisher ID #766

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

No branches or pull requests

5 participants