Re: [Postfixadmin-devel] attached files for bulk folder

[Sonam P. <s_p...@ru...>]

Hello,
Thank you and I look forward for more recommendations and comments. I 
will upload the files once I incorporate all the suggestions and comments.

Sonam

On 2/9/2012 4:15 AM, Christian Boltz wrote:
> Hello,
>
> Am Mittwoch, 8. Februar 2012 schrieb Sonam Penjor:
>> I am attaching the complete files that I have been working on the bulk
>> email creation for the review.
> Thanks a lot!
>
>> Should you have any inquiries  on the
>> attached files, I would be happy to explain.
>>
>> I will appreciate for the comments, advices and recommendations.
> You will regret this offer ;-))
>
>
> Some questions first:
>
> - Do you have some example CSV files I could use for testing?
>    (That would be easier than creating them myself ;-)
>    Bonus points if you also have some CSV files with invalid data
>    (duplicate usernames, invalid mail addresses, whatever)
> - what's the biggest (in number of lines) CSV file you tested?
> - some documentation/description how the CSV file should look like
>    (allowed columns etc.) would be useful. I'd even place it next to the
>    form in the template file.
> - did I get it right that your only modification to en.lang is the
>    addition of 14 texts after this comment:
>    /* Newly added for the uploading Bulk email ID */
>    (just to avoid I overlooked something, it seems en.lang got some
>    changes since you copied it)
>    I'm attaching an updated en.lang - if you have to do more changes,
>    add more texts etc. please use the attached en.lang as base.
>
> Now to some notes on your code:
> - please don't play around with error_reporting - there's a good reason
>    I have a very strict setting for it on my development machine ;-)
>    I know notices can be annoying sometimes, but often they help to find
>    bugs like typos in a variable name which would be hard to find
>    otherwise
> - your class needs to be named model/UploadCsvFile.php - otherwise the
>    autoloader won't find it
> - better error handling would be nice. For example, if I upload an
>    invalid csv file, I get (only) an error message "Invalid Delimiter".
>    It would be better to display the error message using flash_error()
>    and to display the form again
> - the "Invalid Delimiter" error message seems to be misleading - I
>    uploaded a file with the correct delimiter but wrong column headers
>    (first line: "a,b,c,d") and it complained about "Invalid Delimiter".
>    From reading the code, I'd say you should print out the error message
>    you get from scanCsvfile() instead of just checking for failure.
> - I have attached a patch for create-bulkmailbox.php and
>    model/UploadCsvFile.php with some small changes and inline notes -
>    that's easier than including them in the mail. I also attached the
>    modified files, but reading the patches is probably easier.
> - in the class, please explicitely mark the functions as private or
>    public
>
> So much for the first review ;-)
> The above is what I noticed when reading the code. I didn't test much
> (except with an invalid CSV file ;-)  yet, but the above and the inline
> comments will already give you several ideas what you can improve ;-)
>
> And finally: I hope you aren't shocked too much about all the things I
> found in your code ;-))
>
>
> Regards,
>
> Christian Boltz
>
>
> ------------------------------------------------------------------------------
> Keep Your Developer Skills Current with LearnDevNow!
> The most comprehensive online learning library for Microsoft developers
> is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
> Metro Style Apps, more. Free future releases when you subscribe now!
> http://p.sf.net/sfu/learndevnow-d2d
>
>
> _______________________________________________
> Postfixadmin-devel mailing list
> Pos...@li...
> https://lists.sourceforge.net/lists/listinfo/postfixadmin-devel


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.