| Summary: | Fix C4::Context::Zconn | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Julian Maurice <julian.maurice> |
| Component: | Z39.50 / SRU / OpenSearch Servers | Assignee: | Julian Maurice <julian.maurice> |
| Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | dpavlin, fridolin.somers, gmcharlt, m.de.rooy |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 10344 | ||
| Attachments: |
Bug 11701: Unit tests for C4::Context::Zconn
Bug 11701: Fix C4::Context::Zconn Bug 11701: Fix C4::Context::Zconn Bug 11701: Unit tests for C4::Context::Zconn Bug 11701: Fix C4::Context::Zconn [SIGNED OFF] Bug 11701: Unit tests for C4::Context::Zconn [SIGNED OFF] Bug 11701: Fix C4::Context::Zconn Bug 11701: Unit tests for C4::Context::Zconn Bug 11701: Fix C4::Context::Zconn |
||
|
Description
Julian Maurice
2014-02-06 16:15:06 UTC
Created attachment 25101 [details] [review] Bug 11701: Unit tests for C4::Context::Zconn Created attachment 25102 [details] [review] Bug 11701: Fix C4::Context::Zconn I wonder if this report should not cover the further parameters of Zconn too. What about $auth, $piggyback and $syntax? These could (theoretically) have different values too. In the same way the routine should check if it returns the correct object. If you argue that the current codebase does not use these parameters, I probably agree glancing through some grep results. But then we should better remove these parameters and give them default values instead of pretending to support them and at the same time ignoring them just as we do for async until now.. Could you please have a look at that? Created attachment 25439 [details] [review] Bug 11701: Fix C4::Context::Zconn Support other parameters too A quick grep shows me that other parameters are not used, but it isn't harder to support all parameters. I get merge conflict when trying to apply this patch: Applying: Bug 11701: Unit tests for C4::Context::Zconn Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging t/Context.t CONFLICT (content): Merge conflict in t/Context.t Failed to merge in the changes. Patch failed at 0001 Bug 11701: Unit tests for C4::Context::Zconn Would you rebase it on top of master please? Created attachment 26328 [details] [review] Bug 11701: Unit tests for C4::Context::Zconn Rebased on master Created attachment 26329 [details] [review] Bug 11701: Fix C4::Context::Zconn Rebased on master Created attachment 26361 [details] [review] [SIGNED OFF] Bug 11701: Unit tests for C4::Context::Zconn TEST PLAN --------- 1) apply patch 2) run koha qa test tool 3) prove -v t/Context.t Unit tests ran as expected. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26362 [details] [review] [SIGNED OFF] Bug 11701: Fix C4::Context::Zconn When calling C4::Context::Zconn twice with different parameters, the same ZOOM::Connection object is returned twice (parameters of 2nd call are not used) This patch fixes that. This is in part, because the connection cache is keyed on server name only. This patch corrects this by keying on all parameters. TEST PLAN --------- 1) apply patch 2) run koha qa test tools 3) prove -v t/Context.t The unit tests properly triggers the modified routine for testing. Additionally, in hunting for ways it could break, no nested synchronous or asynchronous Zconn's were found. And even if they were, the keying on all parameters should allow it to function properly without messing up the other connection. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 26506 [details] [review] Bug 11701: Unit tests for C4::Context::Zconn TEST PLAN --------- 1) apply patch 2) run koha qa test tool 3) prove -v t/Context.t Unit tests ran as expected. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26507 [details] [review] Bug 11701: Fix C4::Context::Zconn When calling C4::Context::Zconn twice with different parameters, the same ZOOM::Connection object is returned twice (parameters of 2nd call are not used) This patch fixes that. This is in part, because the connection cache is keyed on server name only. This patch corrects this by keying on all parameters. TEST PLAN --------- 1) apply patch 2) run koha qa test tools 3) prove -v t/Context.t The unit tests properly triggers the modified routine for testing. Additionally, in hunting for ways it could break, no nested synchronous or asynchronous Zconn's were found. And even if they were, the keying on all parameters should allow it to function properly without messing up the other connection. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Pushed to master. Thanks, Julian! Patch does not apply on 3.14.x. Since its a test, it's not a big deal. It was easy to rebase so : Pushed to 3.14.x, will be in 3.14.08 |