Bug 15585 - Move C4::Passwordrecovery to Koha::Patron::Password::Reset
Summary: Move C4::Passwordrecovery to Koha::Patron::Password::Reset
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low blocker (vote)
Assignee: Charles Farmer
QA Contact: Testopia
URL:
Keywords:
Depends on: 8753
Blocks: 18956
  Show dependency treegraph
 
Reported: 2016-01-15 14:55 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15585 - Move C4::Passwordrecovery to the new namespace Koha::Patron::Password::Reset (9.99 KB, patch)
2016-01-27 20:33 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 15585 - Move C4::Passwordrecovery to the new namespace Koha::Patron::Password::Reset (10.04 KB, patch)
2016-03-07 19:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15585 - Move C4::Passwordrecovery to the new namespace Koha::Patron::Password::Reset (10.12 KB, patch)
2016-03-22 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15585: Remove "shift on reference is experimental" warning (1.33 KB, patch)
2016-03-22 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-01-15 14:55:52 UTC
Bug 8753 introduces a new C4 module. This module should be moved to the Koha namespace. It looks like Koha::Patron::Password::Reset would be the most appropriate namespace, as the feature allows for password resets rather than being able to recover the original password.
Comment 1 Blou 2016-01-15 18:57:01 UTC
We'll hold our side of the bargain.  I've already created a local task at high priority.   But if it's ok with everyone, we'll wait until 8753 is pushed to master before doing so, just to avoid some rebase.
Comment 2 Blou 2016-01-27 14:09:01 UTC
See bug title and these other corrections demanded on #8753

---
    Minor points:
    Spotted "FIXME: That ugly shift-grep contraption".
    The term "Forgotten password recovery"  sounds funny to me (no native speaker :) Is Password recovery not sufficient?
    Are dots in the unique key (especially at the end) user friendly? 
----
Comment 3 Charles Farmer 2016-01-27 20:33:12 UTC Comment hidden (obsolete)
Comment 4 Charles Farmer 2016-01-27 20:38:53 UTC
Please confirm that Kyle's latest attachment in #8753 fixes what Marcel de Rooy spoke about (missing notice in languages).
Comment 5 Katrin Fischer 2016-01-27 23:11:44 UTC
I just tested Kyle's follow-up for the notices - that should be taken care of. Hint: there is a test to catch that problem - xt/sample_notices.t  :)
Comment 6 Blou 2016-02-29 13:37:04 UTC
Well, just a ping to remind everyone how important the followup was to #8753...  It's here, waiting, alone, probably crying...
Comment 7 Kyle M Hall 2016-03-07 19:21:47 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2016-03-07 19:23:18 UTC
Marking this as a blocker because we really shouldn't allow the next major release to go out without this patch.
Comment 9 Jonathan Druart 2016-03-08 09:32:40 UTC
Kyle,
The module has been moved to the Koha namespace but does not respect the requirements for that (object oriented). I don't see the point to have it in Koha in that case.
Comment 10 Kyle M Hall 2016-03-08 12:46:59 UTC
(In reply to Jonathan Druart from comment #9)
> Kyle,
> The module has been moved to the Koha namespace but does not respect the
> requirements for that (object oriented). I don't see the point to have it in
> Koha in that case.

Koha namespace modules do not *have* to be OO, though they should be.

PERL15:
Whenever it makes sense, code added to the Koha:: namespace should be object-oriented. However, code that is naturally procedural should not be shoehorned into the OO style.

I'd rather see this moved to the Koha namespace and get maybe updated to be OO later than to introduce a new module to C4. After all, why did we get a promise from Charles to submit this patch if we were later going to decide we would not use it!
Comment 11 Blou 2016-03-08 13:49:24 UTC
Interesting, that's the first explanation I have about C4 vs Koha namespace.  I always thought C4 was "old crap" and Koha was "new better".

Still, I don't understand the obsession toward OOing everything, especially in a non-OO language like Perl.  Java created many obsessive behaviors and even more ugly projects because of its misunderstood OO paradigm.  (I'm not calling our QA as having OCD, btw :-)

I'll leave my 3 cents on this marvelous must-read article from an all-time great: http://www.drdobbs.com/cpp/how-non-member-functions-improve-encapsu/184401197 just because I like to plug it :-)
Comment 12 Katrin Fischer 2016-03-08 13:51:22 UTC
I think we left the door open for non-OO modules atm - so I think this one should be ok from the phrasing in the coding guidelines.
Comment 13 Marcel de Rooy 2016-03-09 13:15:34 UTC
(In reply to Blou from comment #11)
> Still, I don't understand the obsession toward OOing everything, especially
> in a non-OO language like Perl.  Java created many obsessive behaviors and
> even more ugly projects because of its misunderstood OO paradigm.  (I'm not
> calling our QA as having OCD, btw :-)
Obsessive Compulsive Disorder ? Interesting. You still need the QA team on this report btw.. :)
Comment 14 Blou 2016-03-09 13:42:41 UTC
Hey, I DID put a smiley :).  Should have winked it, I knew it!

But *I* do not need the QA on this one.  Technically, my patch was #8753, this is just pure generosity toward the QA, actually. :-)  (see, again :)

Cheers!
(Aside: I do believe that a good QA *must* be relentless and dare I say 'obsessive', and Devs must accept some frustration because good QAs are rare and very valuable to any organisation).
Comment 15 Marcel de Rooy 2016-03-09 13:55:53 UTC
(In reply to Blou from comment #14)
> Hey, I DID put a smiley :).  Should have winked it, I knew it!
Of course. But could not resist to react on an abbrev which I was not familiar with btw :) Dutch_abbreviations++
Comment 16 Jonathan Druart 2016-03-22 11:05:18 UTC
Created attachment 49411 [details] [review]
Bug 15585 - Move C4::Passwordrecovery to the new namespace Koha::Patron::Password::Reset

As promised, here is the long-awaited sequel to #8753.

What has changed :

    - The Koha::Patron::Password::Reset is now used in place of C4::Passwordrecovery
    - That ugly shift-grep contraption is no more (goodbye old friend)
    - The generated unique key won't end in a dot anymore

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2016-03-22 11:05:22 UTC
Created attachment 49412 [details] [review]
Bug 15585: Remove "shift on reference is experimental" warning

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2016-03-22 11:08:02 UTC
I personaly don't think this package should be moved yet: it contains SQL queries and use Crypt::Eksblowfish::Bcrypt (should not be done outside of Koha::AuthUtils imo).
But it works, so let's do that later...
Comment 19 Brendan Gallagher 2016-03-22 23:09:23 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 20 Julian Maurice 2016-03-23 06:24:49 UTC
Does not concern stable versions, status changed to RESOLVED