#104 preliminary code to merge vacation

closed-accepted
nobody
Vacation (23)
5
2013-12-01
2011-07-31
J.Kruis
No

this patch i use to get code in to sync toget later one template and one code voor edit-vacation and user-vacation

regarde Jan Kruis

Discussion

  • J.Kruis

    J.Kruis - 2011-07-31

    vacation to merge

     
  • Christian Boltz

    Christian Boltz - 2011-08-01

    I just had a quick look at your patch - looks good :-) - but as usual, I found "a hair in the soup" (as we germans say) ;-)

    In edit-vacation.php, you use (for $tSubject and $tBody)
    $smarty->assign ('tSubject', htmlentities ($tSubject, ENT_QUOTES, 'UTF-8'), false);
    You can most probably use this instead:
    $smarty->assign ('tSubject', $tSubject);
    (without the "false" as third parameter, $smarty->assign does the htmlentities() call itsself - see assign() and sanitize() in smarty.inc.php)
    Or did I overlook something?

    What's your preferred way to continue? Shall I commit your preliminary patch to SVN now or should I wait until you provide a non-preliminary patch? (I'd prefer to commit now, but it's your choice.)

     
  • J.Kruis

    J.Kruis - 2011-08-01

    Hi Christian,

    dont commit the patch it nown
    i upload a second patch with handel the return and removed some unused code in templates
    please have a look and i see up to remarks

    i have a look at the $smarty->assign remark

    reards jan

     
  • J.Kruis

    J.Kruis - 2011-08-01

    update of vacation to merge

     
  • J.Kruis

    J.Kruis - 2011-08-01

    Christian,

    In my attempt to merge edit-vacation and user-vacation vacation together into one file
    (meaning to get the preliminary code and templates as much in sync as possible to obtain unambiguous)
    I noticed something strange.

    In the template edit vacation.tpl I particularly "control" and "name" changed from activeform to fActiveForm.

    I open edit-vacation.php through the edit-list-virtual.php window; everything works well, now I open a second window and open user-vacation everything this still works well, until I reload window 1 (edit vacation).
    Now change the window in the window of user-vacation but call the right url in the browser, that of edit-vacation.php (with the right name and domain).

    Could this be the result of my changes of that review, and particularly name?

    Regards, Jan Kruis

     
  • J.Kruis

    J.Kruis - 2011-08-01

    Hi Christian,

    I have upload new patch.

    In this one i replaced eidt-vaction.tpl and user_vacation.tpl with a new template vacation.tpl
    as a frist step to merge the two module into one.

    what a like to do is put the of the email for with the vacation is change into header somethink link

    auto response for : user@domain.org

    as well user setting as for the admin setting.

    as you can see i also make a patch voor the languages/nl.lang

    If you like the patch sofar you may commit vacatiion-3.patch

    thing i have todo.

    - remove entry in the language with are not used any more.
    - merge edit-vacation.php and user/vaction.php into one file.
    - add useremail to flash-message sure for admin if coming back to list-virtual.php.

    BTW
    I had a look a the $smarty->assign remark, and you were right. It is change in vacation-3.patch.

    if you have any comments or recommendations please let me know

    Regards Jan Kruis

     
  • J.Kruis

    J.Kruis - 2011-08-01

    Hi Christian,

    I have upload new patch.

    In this one i replaced eidt-vaction.tpl and user_vacation.tpl with a new template vacation.tpl
    as a frist step to merge the two module into one.

    what a like to do is put the of the email for with the vacation is change into header somethink link

    auto response for : user@domain.org

    as well user setting as for the admin setting.

    as you can see i also make a patch voor the languages/nl.lang

    If you like the patch sofar you may commit vacatiion-3.patch

    thing i have todo.

    - remove entry in the language with are not used any more.
    - merge edit-vacation.php and user/vaction.php into one file.
    - add useremail to flash-message sure for admin if coming back to list-virtual.php.

    BTW
    I had a look a the $smarty->assign remark, and you were right. It is change in vacation-3.patch.

    if you have any comments or recommendations please let me know

    Regards Jan Kruis

     
  • J.Kruis

    J.Kruis - 2011-08-01

    update of vacation (beta version)

     
  • Christian Boltz

    Christian Boltz - 2011-08-04

    Translation update commited to SVN r1162.

    I'll review the vacation-related parts of your patch tonight.

     
  • Christian Boltz

    Christian Boltz - 2011-08-05

    Some small notes on vacation-3.patch:

    edit.vacation.php + users/vacation.php:
    - The fChange and fBack POST variables are only used with empty() - that means escape_string() is superfluous (but doesn't hurt)
    - I don't see a real need for using $Flash_Message - you can use flash_info()/flash_error() with a $PALANG text directly

    vacation.tpl:
    - displaying the mail address in users/ is superfluous IMHO - I'd propose to wrap this table row in {if !$authentication_has_role.user} ... {/if}

    I nevertheless commited your patch to SVN r1163 (without changes to avoid merge conflicts for you).

    About your TODO list:
    > - remove entry in the language with are not used any more.

    Just tell me which $PALANG texts are no longer needed, I will handle the changes in *.lang

    > - add useremail to flash-message sure for admin if coming back to
    > list-virtual.php.

    Sounds like a good idea. You can use sprintf() and '%s' in the $PALANG text to make the text a valid sentense. Feel free to modify the existing text in the languages you understand. For the others, we can just append '(%s)' to the text and add a comment to the translators that they should make the translation nicer. Again, I can help with updating *.lang if you want - doing this with language-update.sh might be faster than manual editing.

     
  • J.Kruis

    J.Kruis - 2011-08-06

    Hi Christian,

    I used your vacation.tpl adjustment.

    Q1
    I wonder whether it is advisable to rename the languages ​​of [pUsersVacation] change to [pVacation]
    It clearness would clearly benefit, but I think it will generate lot of translation work . I like to hear your opinion.

    Q2
    Flash_Message I wanted to use the email user to be added to the flash_info / flash_error () I try it now via% s see question below.

    Another question relates to sprintf () and% s $ PALANG text?
    where can I find an example of the use of it. I found something in footer.php but did not it dn't call the function with flash or flash error information. when i try it it dn't repleace the %s but show it.

    The adjustment that I will make in the long file *. I will make in the NL and EN version.

    Q3
    I run into another problem.

    Depending on the role (admin or user) I set the variable

    $ return_url = 'list-virtual.php? fDomain $ domain =';
    or
    $ return_url = 'main.php';

    I like the following line ... insert vaction.php they do not work. I get a blank screen.

    header ("Location:". "return_url $ '.)

    question:
    How do I header () returns the location if I want to define as a variable.
    This is the code I currently use.

    [code]
    // only allow admins to change someone else's 'stuff'
    if(authentication_has_role('admin')) {
    $Admin_Role = 1 ;
    $Return_url = 'list-virtual.php?domain=$fDomain';

    if (isset($_GET['username'])) $fUsername = escape_string ($_GET['username']);
    if (isset($_GET['domain'])) $fDomain = escape_string ($_GET['domain']);
    }
    else {
    $Admin_Role = 0 ;
    $Return_url = 'main.php';
    // authentication_require_role('user');
    $fUsername = $SESSID_USERNAME;
    $fDomain = $USERID_DOMAIN;
    }

    // is vacation support enabled in $CONF ?
    if($CONF['vacation'] == 'NO') {
    if ($Admin_Role == 1) {
    header ("Location: list-virtual.php?domain=$fDomain");
    }
    else {
    header ("Location: main.php");
    }
    exit(0);
    }
    [/code]

    Regards,
    Jan Kruis

     
  • Christian Boltz

    Christian Boltz - 2011-08-06

    Q1: Renaming $PALANG texts is not a real problem - language-update.sh can do that (no translations lost). The more interesting question is how to name the variables - IMHO they don't need to have "vacation" in their name. For example, pUsersVacation_subject (text: "Subject") could just be $PALANG[subject] and could also be used in broadcast-message.php (instead of pBroadcast_subject).

    I can do the renaming if you tell me which variables you want to rename and which name you'd like to have.

    Q2: fetchmail.php contains an example:
    flash_info(sprintf($PALANG['pDelete_delete_success'],$account));

    Q3
    > $ return_url = 'list-virtual.php? fDomain $ domain =';

    This should probably be $return_url = 'list-virtual.php?domain=' . $fDomain; (Note: I didn't check the parameter and variable names)

    > I like the following line ... insert vaction.php they do not work. I get a blank screen.

    That's means you should check your error_log ;-) - it usually contains an error message in this case.

    > header ("Location:". "return_url $ '.)

    Shouldn't that be header("Location: $Return_url"); ?

    > $Return_url = 'list-virtual.php?domain=$fDomain';

    You should use "..." quotes instead of '...' (your version won't replace $fDomain with the variable content).

    BTW: You have probably noticed that the SF tracker is not the perfect place for pasting code. Feel free to upload it as attachment or switch to another communication channel, for example the postfixadmin-devel mailinglist.

     
  • J.Kruis

    J.Kruis - 2011-08-07

    last test version ( i hope )

     
  • J.Kruis

    J.Kruis - 2011-08-07

    Hi Christian,

    I think I'm done fairly.

    modules in the patch are not longer used this way can

    edit-vacation.php and users/vacation.php will be replaced by vacation.php .

    templates/edit-vacation.tpl and templates/user-vacatio.tpl wil be replaced by templates/vacation.tpl .

    I ask myself if i used the right way to call vacation voor the users because i al sop saw a reference to url_user_vacation in configs/menu.conf and in templates/users_menu.tpl
    or are these obsolete ?

    I pleaced some remark in the en.lang file .these are the data which we can to put together and could rename. If this will not give any problems with the translations.

    BTW
    I saw in the nl.lang file a few times the same text wand it was not marked as ​obsulte like other text .
    These are among the text of *_password_text_error.

    I uploaded vacation-6.patch.

    Please let me know what you think and want shoud be changed

    Regrades Jan

     
  • Christian Boltz

    Christian Boltz - 2011-08-14

    > I ask myself if i used the right way to call vacation voor the users
    > because i al sop saw a reference to url_user_vacation in configs/menu.conf
    > and in templates/users_menu.tpl
    > or are these obsolete ?

    They are not obsolete, but they are not used everywhere :-(

    Besides that, things aren't as easy as "change a link" because that makes the other links in the users menu link to the wrong place. Instead, I commited a dummy users/vacation.php that only does a require('../vacation.php'). That avoids the link hell ;-)

    > I pleaced some remark in the en.lang file .these are the data which we can
    > to put together and could rename. If this will not give any problems with
    > the translations.

    I've postponed this part of your patch.

    > I uploaded vacation-6.patch.

    I integrated it in svn trunk (with some trouble - looks like you didn't create the patch against latest svn, but it finally worked) and commited it to SVN r1169.

    Please see the commit log message for some details about the differences between your patch and what I commited. (I hope you don't get too many merge conflicts on your next "svn up" run ;-)

    Oh, BTW: Please do not add more variables to variables.inc.php - it's a(nother) leftover I want to get rid of (by initializing the variables in the/each script that requires them).

    From my POV the merge is finished. The only things I noticed while proofreading your changes are:
    - the "vacation already active" message would also be useful for admins, but that probably needs a small text change
    - in admin mode, $fDomain should be read from $fUsername instead of using an url parameter
    - in admin mode, there should be a check if $fUsername is valid
    - (and of course I still have to check your comments in en.lang)

    Those are mostly unrelated to your patch. I won't complain if you fix them, but it's not a real problem for me to fix them myself. (That's especially true for the text change - I can update all language files (well, add a comment) with one script call.)

    If you provide a patch for the above things, please make sure to use the latest svn revision before running "svn diff".

    All that said: thanks for working on this patch!

     
  • J.Kruis

    J.Kruis - 2011-08-21

    Hi Christian,

    I read your commit log message and I agree with the adjustment.
    With the merge of the file i had no problems. I've removed the file from my system and enter a "svn up".

    What I found strange that you had trouble with the merge, because the "svn diff" have taken on him for svn svn 1167 and 1168 have uploaded.

    I will not add more variables to variables.inc.php

    Just coming back to the comment:

    >
    > From my POV the merge is finished. The only things I noticed while
    > proofreading your changes are:
    > the "vacation already active" message would also be useful for admins,
    > but that probably needs a small text change
    >

    I had the message already included in the update but I thought later, because the admin of the list-virutal.php already can see that the vacation message is active. "Set Vacation" and "VACATION IS ON"

    >
    > in admin mode, $fDomain should be read from $fUsername instead of using an url parameter
    >

    [Code]
    // only allow admins to change someone else's 'stuff'
    if(authentication_has_role('admin')) {
    $Admin_role = 1 ;

    if (isset($_GET['username'])) $fUsername = escape_string ($_GET['username']);
    if (isset($_GET['domain'])) $fDomain = escape_string ($_GET['domain']);
    $Return_url = "list-virtual.php?domain=$fDomain";
    }
    else {
    $Admin_role = 0 ;
    # $Return_url = "users/main.php";
    $Return_url = "main.php";
    authentication_require_role('user');
    $fUsername = $SESSID_USERNAME;
    $fDomain = $USERID_DOMAIN;
    }
    [/Code]

    exchange with

    [Code]
    // only allow admins to change someone else's 'stuff'
    if(authentication_has_role('admin')) {
    $Admin_role = 1 ;

    if (isset($_GET['username'])) $fUsername = escape_string ($_GET['username']);
    // if (isset($_GET['domain'])) $fDomain = escape_string ($_GET['domain']);
    $tmp = preg_split ('/@/', $fUsername);
    $fDomain = $tmp[1];
    $Return_url = "list-virtual.php?domain=$fDomain";
    }
    else {
    $Admin_role = 0 ;
    // $Return_url = "users/main.php";
    $Return_url = "main.php";
    authentication_require_role('user');
    $fUsername = $SESSID_USERNAME;
    // $fDomain = $USERID_DOMAIN;
    $tmp = preg_split ('/@/', $fUsername);
    $fDomain = $tmp[1];
    }
    [/Code]

    >
    > in admin mode, there should be a check if $fUsername is valid
    >

    Check for the $fUsername do you think of a check against the database or to a syntax check. I see no benefit in a check against the database. to perform list because vacation.php come from virtual domain and user name comes from the database.

    > (and of course I still have to check your comments in en.lang)

    > Those are mostly unrelated to your patch. I won't complain if you fix
    > them, but it's not a real problem for me to fix them myself. (That's
    > especially true for the text change - I can update all language files
    > (well, add a comment) with one script call.)

    The adjustment in the code I will have for my account, I will leave the language, adapting to your.

     
  • Christian Boltz

    Christian Boltz - 2011-08-21

    > What I found strange that you had trouble with the merge, because the "svn
    > diff" have taken on him for svn svn 1167 and 1168 have uploaded.

    I also found it strange, but well, sh** happens ;-)

    >> the "vacation already active" message would also be useful for admins,
    >> but that probably needs a small text change
    >
    > I had the message already included in the update but I thought later,
    > because the admin of the list-virutal.php already can see that the vacation
    > message is active. "Set Vacation" and "VACATION IS ON"

    basically correct, but having it on the vacation page won't hurt ;-)

    >> in admin mode, $fDomain should be read from $fUsername instead of using
    >> an url parameter

    > [Code] ...

    Yes, something like that, but completely different ;-) See r1172 if you are interested what I did - (I included some additional changes and cleanup)

    > > in admin mode, there should be a check if $fUsername is valid

    also part of r1172, at least as a basic check

    > Check for the $fUsername do you think of a check against the database or
    > to a syntax check. I see no benefit in a check against the database. to
    > perform list because vacation.php come from virtual domain and user name
    > comes from the database.

    General rule of thumb: never trust your users ;-)
    The username is handed over as URL parameter, and an evil user could always modify it.

    > The adjustment in the code I will have for my account, I will leave the
    > language, adapting to your.

    I just added the obsolete markers to the language files (r1170).

    I still have to rename the remaining/used texts to something more useful - but that's something that should be done for all texts, not only the vacation-related texts.

    Nevertheless I'll close this tracker item as fixed :-)

     
  • Christian Boltz

    Christian Boltz - 2011-08-21
    • status: open --> closed-accepted
     

Log in to post a comment.