postfixadmin-devel

[Postfixadmin-devel] Bug in v3.0 vacation module

[Ullrich v. B. <uz...@mu...>]

Good evening!

I wasn't able to open a ticket so here is a small diff that fixes a problem
with the vacation module. It references a table column that seems to have
existed in older versions, but does no longer in version 3. I'm using this
package from the web site: postfixadmin-3.0.fedora.noarch.rpm

-------------------------------------------------------------------------------
--- VacationHandler.orig	2016-05-22 18:17:46.000000000 +0200
+++ VacationHandler.php	2016-12-13 15:50:18.664179212 +0100
@@ -21,7 +21,6 @@
             'body'          => pacol(   1,          1,      0,      'text', 'pUsersVacation_body'           , ''                                , '' ),
             'activefrom'    => pacol(   1,          1,      1,      'text', 'pUsersVacation_activefrom'     , ''                                , '' ),
             'activeuntil'   => pacol(   1,          1,      1,      'text', 'pUsersVacation_activeuntil'    , ''                                , '' ),
-            'cache'         => pacol(   0,          0,      0,      'text', ''                              , ''                                , '' ), # leftover from 2.2
             'active'        => pacol(   1,          1,      1,      'bool', 'active'                        , ''                                 , 1 ),
             'created'       => pacol(   0,          0,      1,      'ts',   'created'                       , ''                                 ),
             'modified'      => pacol(   0,          0,      1,      'ts',   'last_modified'                 , ''                                 ),
@@ -180,7 +179,6 @@
             'active' => db_get_boolean(true),
             'activefrom' => $activeFrom,
             'activeuntil' => $activeUntil,
-            'cache' => '',
         );

         // is there an entry in the vacaton table for the user, or do we need to insert?
-------------------------------------------------------------------------------

I hope it is ok to post it here. If not, please let me know.

There seems to be a another problem with display of date strings in a few
places, which I will have to investigate.

Regards


        Uz


-- 
Ullrich von Bassewitz                                  uz...@mu...
Encrypted email preferred                          PGP Key-Id: 29D93B10

Re: [Postfixadmin-devel] Bug in v3.0 vacation module

[David G. <da...@co...>]

Hi -

Thanks - applied in changeset 1883

David.

On 19/12/2016 19:40, Ullrich von Bassewitz wrote:
>
> Good evening!
>
> I wasn't able to open a ticket so here is a small diff that fixes a problem
> with the vacation module. It references a table column that seems to have
> existed in older versions, but does no longer in version 3. I'm using this
> package from the web site: postfixadmin-3.0.fedora.noarch.rpm
>
> -------------------------------------------------------------------------------
> --- VacationHandler.orig	2016-05-22 18:17:46.000000000 +0200
> +++ VacationHandler.php	2016-12-13 15:50:18.664179212 +0100
> @@ -21,7 +21,6 @@
>              'body'          => pacol(   1,          1,      0,      'text', 'pUsersVacation_body'           , ''                                , '' ),
>              'activefrom'    => pacol(   1,          1,      1,      'text', 'pUsersVacation_activefrom'     , ''                                , '' ),
>              'activeuntil'   => pacol(   1,          1,      1,      'text', 'pUsersVacation_activeuntil'    , ''                                , '' ),
> -            'cache'         => pacol(   0,          0,      0,      'text', ''                              , ''                                , '' ), # leftover from 2.2
>              'active'        => pacol(   1,          1,      1,      'bool', 'active'                        , ''                                 , 1 ),
>              'created'       => pacol(   0,          0,      1,      'ts',   'created'                       , ''                                 ),
>              'modified'      => pacol(   0,          0,      1,      'ts',   'last_modified'                 , ''                                 ),
> @@ -180,7 +179,6 @@
>              'active' => db_get_boolean(true),
>              'activefrom' => $activeFrom,
>              'activeuntil' => $activeUntil,
> -            'cache' => '',
>          );
>
>          // is there an entry in the vacaton table for the user, or do we need to insert?
> -------------------------------------------------------------------------------
>
> I hope it is ok to post it here. If not, please let me know.
>
> There seems to be a another problem with display of date strings in a few
> places, which I will have to investigate.
>
> Regards
>
>
>         Uz
>
>

Re: [Postfixadmin-devel] Bug in v3.0 vacation module

[Christian B. <pos...@cb...>]

Hello,

Am Montag, 19. Dezember 2016 schrieb Ullrich von Bassewitz:
> I wasn't able to open a ticket so here is a small diff that fixes a
> problem with the vacation module. It references a table column that
> seems to have existed in older versions, but does no longer in
> version 3. I'm using this package from the web site:
> postfixadmin-3.0.fedora.noarch.rpm

Which database do you use? MySQL, PostgreSQL or SQLite?
(I'd guess PostgreSQL ;-)


I'm asking because I'm quite sure [1] your patch broke the vacation 
module for MySQL users, at least those using strict mode. The "cache" 
column is a text (not varchar) field which doesn't allow to define a 
default value, therefore we need to specify a value as long as this 
column exists.

The "cache" field was used by old (MySQL only [2]) versions of 
vacation.pl (to store what we now have in the vacation_notification 
table), so the question is how we should continue.

The reason why I kept the column is that people out there might still 
(accidently) use an old vacation.pl together with the new PostfixAdmin. 
So the possible options are:
a) drop the column (and hope that not too many people forgot to update
   to the new vacation.pl)
b) add the column for all database types (yes, that sounds pointless,
   but would avoid differences in the database structure)
c) change VacationHandler so that it only knows and fills the cache 
   column if using a MySQL database

Opinions? ;-)


Regards,

Christian Boltz

[1] I know the code good enough to be quite sure ;-)) - but I didn't
    test it.

[2] Back then, we had a MySQL vacation.pl and a PostgreSQL vacation.pl.
    David then merged both into one, using the PostgreSQL vacation.pl as
    base. IIRC the PostgreSQL version of vacation.pl never used the 
    cache field, which explains why it doesn't exist in the PostgreSQL 
    database layout.

-- 
> IIRC gab es ÜHER mal so seltsame rechteckige Blöcke mit vielen
> einzelnen Blättern aus Zellulosefasern in denen man Rezepte nachlesen
> konnte... aber frag' mich jetzt nur nicht, wie die hießen.
Du redest doch wohl nicht von ermordeten Bäumen?
[>T. Braun und S. Posner]

Re: [Postfixadmin-devel] Bug in v3.0 vacation module

[Ullrich v. B. <uz...@mu...>]

[Send this email to Christian not realizing that the emails from the list
don't have a Reply-To header that points to the list. I'll try to be more
careful in the future.]

Good morning!

On Thu, Dec 22, 2016 at 01:23:32AM +0100, Christian Boltz wrote:
> Which database do you use? MySQL, PostgreSQL or SQLite?
> (I'd guess PostgreSQL ;-)

I have both, a postgresql setup for production and a mysql setup for testing.
The new vacation module works with the mysql setup without problems. I haven't
changed anything in the mysql installation, so I don't know if strict mode is
enabled or not.

> I'm asking because I'm quite sure [1] your patch broke the vacation
> module for MySQL users, at least those using strict mode. The "cache"
> column is a text (not varchar) field which doesn't allow to define a
> default value, therefore we need to specify a value as long as this
> column exists.

The existing vacation module does definitely not work with postgres. With my
patch, it works with postgres and with mysql (maybe not in strict mode). So I
would say, it's at least an improvement :)

> Opinions? ;-)

If the only purpose of the column is to let people run older versions of the
vacation module, then I would definitely suggest to drop it. Keeping old cruft
like this in each version will kill you in the long run. That's my two cent.
Your opinion may of course be different. :)

Regards


        Uz


-- 
Ullrich von Bassewitz                                  uz...@mu...
Encrypted email preferred                          PGP Key-Id: 29D93B10

Re: [Postfixadmin-devel] Bug in v3.0 vacation module

[Tanstaafl <tan...@li...>]

On 12/21/2016 7:23 PM, Christian Boltz <pos...@cb...> wrote:
> a) drop the column (and hope that not too many people forgot to update
>    to the new vacation.pl)

I think spending your valuable time trying to foresee *and* resolve
other people's mistakes is a really bad way to handle things.

Just make sure the upgrade path is fully documented, and leave people
free to learn from their mistakes.