[Koha-bugs] [Bug 17427] Replace CGI::Session with Data::Session

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427

--- Comment #92 from Jonathan Druart <jonathan.druart+koha at gmail.com> ---
(In reply to David Cook from comment #91)
> (In reply to Jonathan Druart from comment #90)
> > (In reply to David Cook from comment #89)
> > > What's outstanding on this one?
> > 
> > Read the commit messages.
> 
> I did and I couldn't make sense of them.

Ok, trying to be more verbose then.

1. For discussion, waiting for review and feedback:
(In reply to Jonathan Druart from comment #82)
> Created attachment 150929 [details] [review]
> Bug 17427: HACKY - force new session
> 
> Look at Data::Session->new, it's calling $self -> load_session;
> Then look at load_session then user_id
> => we are retrieving the ID from the cookie
> 
> But in our Auth.t tests we are mocking CGI->cookie to always return the
> previous CGISESSID
> 
> We should certainly adjust the test here, and remove this patch.
> 
> 194         # Note: We can test return values from checkauth here since we
> mocked the safe_exit after the Redirect 303
> 195         is( $return[0], $patron2->userid, 'Login of patron2 approved' );
> 196         isnt( $return[2], $sessionID, 'Did not return previous session
> ID' );
> 197         ok( $return[2], 'New session ID not empty' );

What do you think about this, should we adjust the test?

2. The behaviour is buggy, see the following comment:

(In reply to Jonathan Druart from comment #86)
> (In reply to Jonathan Druart from comment #73)
> > Created attachment 150638 [details] [review] [review]
> > Bug 17427: Retrieve sessionID from HTTP_COOKIE
> > 
> > To make some tests pass (t/db_dependent/Auth.t) and mimick what did
> > CGI::Session, but that is certainly useless and the tests should be
> > adjusted.
> 
> This patch fixes some tests but make the sessionID handling very wrong. We
> always end up with the same sessionID (even after logout, change user, etc.)
> 
> I think we should revert it and remove the related tests.

Do you agree that we should remove the related tests?

3. This needs to be fully tested, but first we need to fix the failures and
bugs highlighted in 1 and 2.

-- 
You are receiving this mail because:
You are watching all bug changes.
You are the assignee for the bug.