[ postfixadmin-Patches-3383236 ] preliminary code to merge vacation
Brought to you by:
christian_boltz,
gingerdog
Menu
▾
▴
[ postfixadmin-Patches-3383236 ] preliminary code to merge vacation
|
From: SourceForge.net <no...@so...> - 2011-08-14 23:25:38
|
Patches item #3383236, was opened at 2011-07-31 14:38 Message generated for change (Comment added) made by christian_boltz You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=937966&aid=3383236&group_id=191583 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: Vacation Group: SVN (please specify revision!) Status: Open Resolution: None Priority: 5 Private: No Submitted By: J.Kruis (jan-kruis) Assigned to: Nobody/Anonymous (nobody) Summary: preliminary code to merge vacation Initial Comment: 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 ---------------------------------------------------------------------- >Comment By: Christian Boltz (christian_boltz) Date: 2011-08-15 01:25 Message: > 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! ---------------------------------------------------------------------- Comment By: J.Kruis (jan-kruis) Date: 2011-08-08 01:40 Message: 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 ---------------------------------------------------------------------- Comment By: Christian Boltz (christian_boltz) Date: 2011-08-07 01:31 Message: 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. ---------------------------------------------------------------------- Comment By: J.Kruis (jan-kruis) Date: 2011-08-06 23:17 Message: 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 ---------------------------------------------------------------------- Comment By: Christian Boltz (christian_boltz) Date: 2011-08-05 22:18 Message: 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. ---------------------------------------------------------------------- Comment By: Christian Boltz (christian_boltz) Date: 2011-08-04 13:38 Message: Translation update commited to SVN r1162. I'll review the vacation-related parts of your patch tonight. ---------------------------------------------------------------------- Comment By: J.Kruis (jan-kruis) Date: 2011-08-01 22:02 Message: 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 : us...@do... 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 ---------------------------------------------------------------------- Comment By: J.Kruis (jan-kruis) Date: 2011-08-01 22:02 Message: 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 : us...@do... 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 ---------------------------------------------------------------------- Comment By: J.Kruis (jan-kruis) Date: 2011-08-01 10:51 Message: 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 ---------------------------------------------------------------------- Comment By: J.Kruis (jan-kruis) Date: 2011-08-01 10:44 Message: 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 ---------------------------------------------------------------------- Comment By: Christian Boltz (christian_boltz) Date: 2011-08-01 02:23 Message: 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.) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=937966&aid=3383236&group_id=191583 |