|
Description
Kyle M Hall (khall)
2016-01-15 14:55:52 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. 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?
----
Created attachment 47359 [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 Please confirm that Kyle's latest attachment in #8753 fixes what Marcel de Rooy spoke about (missing notice in languages). 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 :) Well, just a ping to remind everyone how important the followup was to #8753... It's here, waiting, alone, probably crying... Created attachment 48762 [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> Marking this as a blocker because we really shouldn't allow the next major release to go out without this patch. 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. (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! 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 :-) 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. (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.. :) 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). (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++ 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> 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> 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... Pushed to Master - Should be in the May 2016 release. Thanks! Does not concern stable versions, status changed to RESOLVED |