Conversation
- Test a feed with multiple license blocks. - Test a single license block with different numbers of licenses available.
|
@leonardr do these tests illustrate the situation that you were talking about in #25 (comment) ? |
|
@ThePalaceProject/backend tagging you both to see if these tests make sense to you. |
jonathangreen
force-pushed
the
bugfix/odl-multiple-licenses
branch
from
f623e85 to
5ab1a6f
Compare
tdilauro
left a comment
tdilauro
left a comment
There was a problem hiding this comment.
Took a quick look and the logic makes sense. A couple of the comments seem out of sync with the logic and could be confusing.
|
Thanks @tdilauro I updated those comments to make them more clear. |
|
Feel free to just push more commits to this branch if you want to @vbessonov |
| remaining_checkouts = checkouts.get("left") | ||
| available_concurrent_checkouts = checkouts.get("available") | ||
|
|
||
| if remaining_checkouts is None: |
There was a problem hiding this comment.
I think that if left property is missing but total_checkouts is there it means that nobody used it so far.
But on the other hand it might mean that it actually expired and lack of left means that no checkouts left.
There was a problem hiding this comment.
If left is missing, it is an error according to the ODL spec. Have we seen that case?
There was a problem hiding this comment.
I put this question to De Marque. Hopefully will have a response soon.
There was a problem hiding this comment.
De Marque indicated that you should not expect to see left when there is an unlimited number of checkouts (which is not consistent with the spec). In that case, total_checkouts should be missing also, as it is in the example you gave above.
Have you seen any cases where left is missing, but total_checkouts is present?
There was a problem hiding this comment.
@tdilauro, yes, there are cases like that in the DPLA Exchange feed. I added a new cell to the Jupyter notebook where all the licenses like that are shown.
|
@jonathangreen, @tdilauro, the changes I made should cover most of the cases except the case of perpetual licenses, i.e. licenses without |
tdilauro
left a comment
tdilauro
left a comment
There was a problem hiding this comment.
This is looking very good. There are a couple of places where some additional documentation would be helpful, as noted.
Regarding the fetch of the license status document, it think it would be helpful to have some visibility into failures when we don't get one when we expect to.
|
I think this is looking ready to merge other then the two unresolved comments left. @leonardr if you have time to give this a review, any comments you have here would be appreciated as well, since you are likely going to pull this fix in upstream as well. |
…ding an LCP License Status Document
| ) | ||
|
|
||
|
|
||
| class TestLicense: |
There was a problem hiding this comment.
This looks like it's mocking the License data model class, which isn't ODL-specific. So this could go into testing.py. But right now the only reason to do it would be if you also needed it in test_odl2.py.
There was a problem hiding this comment.
It's not used in test_odl2.py explicitly but rather implicitly. Test classes in test_odl2.py inherit from base classes defined in test_odl.py.
|
Sorry for the delay. I'm going through this and I think it does a good job of addressing the cases I brought up, so I don't think I'll have any substantive comments. I'm not wild about the jinja2 dependency, but it looks like you introduced that earlier. As long as it's just a test dependency I can deal with it. |
|
Yes, this looks good. It also appears that jinja2 is a dependency of one of our existing dependencies, so that's less of a problem than I thought. |
* Show lcp:hashed_passphrase only for LCP DRM-protected items * Refactor to align with tags on Adobe DRM. Co-authored-by: Jonathan Green <[email protected]>
4 participants
Description
This PR is a work in progress. Its failing currently since its just tests to try to illustrate the problem.
@leonardr made a comment on #25 (#25 (comment)) that there may be some issue in recalculating when a
licensepoolhas multiplelicensesand a single one expires.Motivation and Context
To try to understand the problem that @leonardr indicated, I added a set of tests for OPDS and OPDS2 + ODL that import a feed where a single publication has multiple licenses and where a single license block has more then one license available.