Bug 21233 - Add Koha::Exceptions::Password class
Summary: Add Koha::Exceptions::Password class
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 17006 21178
  Show dependency treegraph
 
Reported: 2018-08-16 09:54 UTC by Tomás Cohen Arazi
Modified: 2019-10-14 19:57 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21233: Unit tests (1.82 KB, patch)
2018-08-16 09:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21233: Add Koha::Exceptions::Password (3.24 KB, patch)
2018-08-16 09:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21233: Unit tests (1.87 KB, patch)
2018-08-16 12:54 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21233: Add Koha::Exceptions::Password (3.30 KB, patch)
2018-08-16 12:54 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21233: Add Koha::Exceptions::Password (3.40 KB, patch)
2018-08-24 09:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21233: Unit tests (1.97 KB, patch)
2018-08-24 09:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21233: (QA follow-up) We do not like the plural form of whitespace (1.33 KB, patch)
2018-08-24 09:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21233: (QA follow-up) It is not only trailing whitespace characters (1.04 KB, patch)
2018-09-14 19:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21233: (follow-up) POD fix (864 bytes, patch)
2018-09-14 23:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2018-08-16 09:54:08 UTC
We need exceptions for passwords.
Comment 1 Tomás Cohen Arazi 2018-08-16 09:58:18 UTC
Created attachment 77880 [details] [review]
Bug 21233: Unit tests

This patch adds tests for the required password-related exceptions. The
tests verify the stringified version of the exceptions.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> FAIL: Exceptions not implemented, tests fail!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2018-08-16 09:58:23 UTC
Created attachment 77881 [details] [review]
Bug 21233: Add Koha::Exceptions::Password

This patch adds some exceptions that we need. To test:

- Apply this patch
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
- Sign off! :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Josef Moravec 2018-08-16 12:54:15 UTC
Created attachment 77914 [details] [review]
Bug 21233: Unit tests

This patch adds tests for the required password-related exceptions. The
tests verify the stringified version of the exceptions.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> FAIL: Exceptions not implemented, tests fail!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 4 Josef Moravec 2018-08-16 12:54:19 UTC
Created attachment 77915 [details] [review]
Bug 21233: Add Koha::Exceptions::Password

This patch adds some exceptions that we need. To test:

- Apply this patch
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
- Sign off! :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Marcel de Rooy 2018-08-24 08:54:44 UTC
FAIL   Koha/Exceptions/Password.pm
   FAIL   pod coverage
                POD is missing for 'description'

This might be a qa tools bug?
podchecker does not warn on this module.
Comment 6 Marcel de Rooy 2018-08-24 08:58:06 UTC
Reversing patch order..
Comment 7 Marcel de Rooy 2018-08-24 09:02:54 UTC
Just a language thing on TrailingWhitespaces
I would say we have trailing spaces or trailing whitespace where whitespace is \s* in regex terms (catching all spaces, tabs, newlines etc.). No plural form needed.

From https://english.stackexchange.com/questions/25368/what-is-the-plural-form-of-whitespace:
The word "whitespace" is usually uncountable. If you need it to be countable, I would suggest saying "whitespace characters".
Comment 8 Marcel de Rooy 2018-08-24 09:04:37 UTC
For some other report:
git grep -l [wW]hitespaces
:)
Comment 9 Marcel de Rooy 2018-08-24 09:08:53 UTC
Created attachment 78116 [details] [review]
Bug 21233: Add Koha::Exceptions::Password

This patch adds some exceptions that we need. To test:

- Apply this patch
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
- Sign off! :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-08-24 09:09:02 UTC
Created attachment 78117 [details] [review]
Bug 21233: Unit tests

This patch adds tests for the required password-related exceptions. The
tests verify the stringified version of the exceptions.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> FAIL: Exceptions not implemented, tests fail!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2018-08-24 09:09:06 UTC
Created attachment 78118 [details] [review]
Bug 21233: (QA follow-up) We do not like the plural form of whitespace

The word "whitespace" is usually uncountable.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Tomás Cohen Arazi 2018-08-24 13:04:25 UTC
(In reply to Marcel de Rooy from comment #5)
> FAIL   Koha/Exceptions/Password.pm
>    FAIL   pod coverage
>                 POD is missing for 'description'
> 
> This might be a qa tools bug?
> podchecker does not warn on this module.

That's a false positive. Yes.
Comment 13 Tomás Cohen Arazi 2018-08-24 13:05:26 UTC
(In reply to Marcel de Rooy from comment #7)
> Just a language thing on TrailingWhitespaces
> I would say we have trailing spaces or trailing whitespace where whitespace
> is \s* in regex terms (catching all spaces, tabs, newlines etc.). No plural
> form needed.
> 
> From
> https://english.stackexchange.com/questions/25368/what-is-the-plural-form-of-
> whitespace:
> The word "whitespace" is usually uncountable. If you need it to be
> countable, I would suggest saying "whitespace characters".

I'd say we should even get rid of 'Trailing', because this is then used in a test for whitespace chars at the begining too, right?
Comment 14 Marcel de Rooy 2018-08-27 06:30:43 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> (In reply to Marcel de Rooy from comment #7)
> > Just a language thing on TrailingWhitespaces
> > I would say we have trailing spaces or trailing whitespace where whitespace
> > is \s* in regex terms (catching all spaces, tabs, newlines etc.). No plural
> > form needed.
> > 
> > From
> > https://english.stackexchange.com/questions/25368/what-is-the-plural-form-of-
> > whitespace:
> > The word "whitespace" is usually uncountable. If you need it to be
> > countable, I would suggest saying "whitespace characters".
> 
> I'd say we should even get rid of 'Trailing', because this is then used in a
> test for whitespace chars at the begining too, right?

Too late ?
Comment 15 Tomás Cohen Arazi 2018-09-14 19:28:21 UTC
Created attachment 78721 [details] [review]
Bug 21233: (QA follow-up) It is not only trailing whitespace characters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2018-09-14 19:28:42 UTC
(In reply to Marcel de Rooy from comment #14)
> (In reply to Tomás Cohen Arazi from comment #13)
> > (In reply to Marcel de Rooy from comment #7)
> > > Just a language thing on TrailingWhitespaces
> > > I would say we have trailing spaces or trailing whitespace where whitespace
> > > is \s* in regex terms (catching all spaces, tabs, newlines etc.). No plural
> > > form needed.
> > > 
> > > From
> > > https://english.stackexchange.com/questions/25368/what-is-the-plural-form-of-
> > > whitespace:
> > > The word "whitespace" is usually uncountable. If you need it to be
> > > countable, I would suggest saying "whitespace characters".
> > 
> > I'd say we should even get rid of 'Trailing', because this is then used in a
> > test for whitespace chars at the begining too, right?
> 
> Too late ?

:-D
Comment 17 Nick Clemens 2018-09-14 19:54:55 UTC
Awesome work all!

Pushed to master for 18.11
Comment 18 Tomás Cohen Arazi 2018-09-14 23:24:14 UTC
Created attachment 78836 [details] [review]
Bug 21233: (follow-up) POD fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Nick Clemens 2018-09-14 23:27:07 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> Created attachment 78836 [details] [review] [review]
> Bug 21233: (follow-up) POD fix
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Follow up pushed to master
Comment 20 Martin Renvoize 2018-09-17 13:42:48 UTC
Enhancement, will not be backported to 18.05.x series.