ODL availability proposal (PP-869) #1903
Conversation
jonathangreen
force-pushed
the
feature/odl-feed-removals
branch
from
85a37db to
031d890
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1903 +/- ##
==========================================
- Coverage 90.28% 90.28% -0.01%
==========================================
Files 332 332
Lines 39873 39868 -5
Branches 8649 8652 +3
==========================================
- Hits 36001 35996 -5
Misses 2577 2577
Partials 1295 1295 ☔ View full report in Codecov by Sentry. |
| available = opds2_ast.OPDS2AvailabilityType.AVAILABLE.value | ||
| if ( | ||
| availability | ||
| and availability.state != available |
There was a problem hiding this comment.
If an availability property is present here, should we also consider the since date/time, if present? The discussion as of the end of late March left this unresolved. Is the availability property in effect if the since date/time has not yet arrived?
There was a problem hiding this comment.
Because since implies past tense, I'm assuming that it is in the past. So since is just informational in the feed. Hadrien seemed strongly against since giving a state change in the future on the OPDS call.
There was a problem hiding this comment.
I can add a check that we fall within the range since -> until if you think that makes more sense. I was just basing the behaviour upon the discussion in the OPDS working group call.
There was a problem hiding this comment.
Fair enough. I'm not super happy with where we left that discussion and don't find compelling the arguments against representing future availability. That said, I agree with your other comment that we don't have enough information for other behavior.
Hadrien's comments there seem to imply that availability.since is, at best, informational (it shouldn't be actioned on for a future change); so, perhaps it is best not to use it in any logic for now.
There was a problem hiding this comment.
I called out that since isn't used in my new comment, and I added a todo to revisit how we handle since once there is some resolution of your question here opds-community/drafts#63 (reply in thread)
| cls, availability: opds2_ast.OPDS2AvailabilityInformation | None | ||
| ) -> bool: | ||
| """Extract the publication's availability from its availability information. | ||
|
|
There was a problem hiding this comment.
Is our stance here that, if an availability is present, but we are past the until date/time, that we simply act as if the availability prop is not present and then just use the default availability behavior?
Might be worth a comment in the docstring here.
Also might be worthwhile to to have a DEFAULT_AVAILABILITY (True for our current behavior) and distinguish between an active availability property and the default more explicitly.
There was a problem hiding this comment.
Yes, that is what my assumption is here. I'll add a doc comment to make it more explicit. I don't think we have enough information for any other behaviour, since we don't have any indication of what the future state would be.
There was a problem hiding this comment.
I added a more detailed comment on the _extract_availability function.
jonathangreen
force-pushed
the
feature/odl-feed-removals
branch
from
51b17ef to
c707399
Compare
Description
This will require ThePalaceProject/webpub-manifest-parser#170 to be merged, and our poetry dependencies to be updated before merging. Thepalace-webpub-manifest-parserdependency is temporarily pointing at a branch to make it easier to test.This PR moves the
time_trackingproperty that was overridden by a custom class in Palace, back out into thewebpub-manifest-parser, and adds support for the draft ODL Availability (removals) proposal here: opds-community/drafts#63Motivation and Context
JIRA: PP-869
This is the first round of work on this. It does not address several cases that were mentioned in the tech strategy meeting last week:
My thinking in that these can be handled in follow up tickets, since this PR solves the immediate need to be able to have items removed from feeds, and now show up as available for new loans.
How Has This Been Tested?
Checklist