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

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

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
-- 
As usual, I'm voicing my opinion on this topic, not announcing "truth" 
;)
[Cristian Rodríguez in opensuse-factory]