Adds new type of dialect for SIP2 servers that require spaces instead… #1495
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1495 +/- ##
=======================================
Coverage 90.38% 90.38%
=======================================
Files 232 232
Lines 29341 29347 +6
Branches 6855 6856 +1
=======================================
+ Hits 26519 26525 +6
Misses 1799 1799
Partials 1023 1023
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
api/sip/client.py
Outdated
| self.dialect_config = dialect.config | ||
|
|
||
| if isinstance(dialect, str): | ||
| self.dialect_config = Dialect(dialect).config |
There was a problem hiding this comment.
This fix here also addresses a problem with the AG Verso dialect. Without this conversion from a string value to the enum an exception will be thrown when trying to access the non-existent property.
There was a problem hiding this comment.
This is a good catch @dbernstein.
I spent a bit of time looking into it, and this is actually a more pervasive issue then just in this integration. Instead of adding the code to deal with the dialect coming in as a string here, I'd rather deal with the up-steam cause so that we get the correct enum type passed in here.
I pushed a new PR here that should do that: #1500
api/sip/__init__.py
Outdated
| @@ -120,6 +120,7 @@ class SIP2Settings(BasicAuthProviderSettings): | |||
| options={ | |||
| Sip2Dialect.GENERIC_ILS: "Generic ILS", | |||
| Sip2Dialect.AG_VERSO: "Auto-Graphics VERSO", | |||
| Sip2Dialect.TZ_SPACES: "TZ Spaces", | |||
There was a problem hiding this comment.
I know there was some discussion of this in the ticket, I think this would be better named Folio then TZ Spaces. I pinged Carissa and Courtney on the ticket for some feedback from their perspective.
api/sip/client.py
Outdated
| self.dialect_config = dialect.config | ||
|
|
||
| if isinstance(dialect, str): | ||
| self.dialect_config = Dialect(dialect).config |
There was a problem hiding this comment.
This is a good catch @dbernstein.
I spent a bit of time looking into it, and this is actually a more pervasive issue then just in this integration. Instead of adding the code to deal with the dialect coming in as a string here, I'd rather deal with the up-steam cause so that we get the correct enum type passed in here.
I pushed a new PR here that should do that: #1500
| now = utc_now() | ||
| return datetime.datetime.strftime(now, "%Y%m%d0000%H%M%S") | ||
| zzzz = " " * 4 if tz_spaces else "0" * 4 | ||
| return datetime.datetime.strftime(now, f"%Y%m%d{zzzz}%H%M%S") |
There was a problem hiding this comment.
Can we add a small test for this that makes sure now() formats the date as we would expect based on the dialect selected?
… of zeros in the time zone segment of the date time values sent via the SIP2 protocol.
b9c80bf to
fa7c6ca
Compare
* Use "FOLIO" rather than "TZ SPACES" as dialect name * Revert dialect type check in sip client since new changes in the main obviate the need for them.
fa7c6ca to
0a75fe1
Compare
|
@jonathangreen : I believe I've addressed your change requests. I believe this one can be merged. |
… of zeros in the time zone segment of the date time values sent via the SIP2 protocol.
Description
Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-546
How Has This Been Tested?
Manually test in addition to the unit tests.
Checklist