Re: [Postfixadmin-devel] Bulk email upload codes - second review

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

Hello,

(sorry for not responding earlier - I really need days with more than 24 
hours...)

Am Donnerstag, 22. März 2012 schrieb s_p...@ru...:
> I am happy to attach the codes for creating bulk email id for the
> second review.
> 
> I will appreciate for your comments, suggestions and advice. Should
> you have any inquires on my works please free to contact me. I will
> be happy to provide an information.

I did some tests this time (not only reading code) and found some bugs:

- the check for the allowed number of mailbox is wrong. You are testing 
  with check_mailbox() - but this function only checks if you are 
  allowed to create _one_ more mailbox on the domain.
  Please add a TODO in your code for now - I'll probably change 
  check_mailbox so that the number of to-be-created mailboxes can be 
  specified

- even if mailbox creation aborts because I hit the maximum number of 
  the allowed mailboxes, I get a success message

- some of your *.csv files come with an invalid header, which (of 
  course) lets them fail

- "Please select Domain" and "Delimiter: :  " are hardcoded in the 
  tpl file - please move them to en.lang (without the " ")

- $PALANG['pCreate_mailbox_csvdomian'] contains a typo (csvdom*ia*n).
  Anyway, please use $PALANG['pOverview_get_domain'] instead - it 
  already exists and is translated.

- when selecting "\t" as delimiter, I get
      Notice: fgetcsv(): delimiter must be a single character in 
      .../model/UploadCsvFile.php on line 86
  I'd guess you don't convert \t to a "real" tab character.


There are also some things that are not bugs, but could be done better:

- displaying errors could be done in a table:
      line number  |  username  |  error message
  IMHO, this will make the list easier readable. It would also allow to
  shorten some texts:
  - $PALANG['pCreate_csvmailbox_errMsg'] =
    'Your uploaded CSV file contains the following errors:';
  - $PALANG['InvalidEmailExisit'] = ' already exists';
  - remove "Error line number:" from all texts - it will be shown in the 
    first column of the table already

- the background colors in the error list are very dark and make the 
  text hard to read. 
  I'm not sure if we really need the background color - I'd just use the 
  "normal" table formatting we have in list-domain etc.

- if all CSV entries are invalid or duplicates, please still display the 
  detailed error messages. Replacing the buttons with the "wrong 
  entries" error message is enough.

- if the upload form is re-displayed because of errors, it should 
  remember the selected domain and delimiter

- coding style: please include the opening curly brackets in the 
  statement's line, for example
      if ($foo == 'bar') {
          # do something
       } else {
          # something else
       }
  There's no need to go through all files just to adjust this - but 
  if you change a code section anyway, please do the formatting this 
  way.

- sometimes you tend to split everything out to functions. While this is 
  often a good idea, sometimes doing it without another (very small) 
  function makes the code easier to understand. getReport() and 
  setErrorMsg() are two examples.

I have also attached a patch with inline comments and small changes for 
UploadCsvFile.php.


That all said: This version looks much better than the files you 
initially sent :-) - and I'll *always* find something to criticize [1], 
so don't worry too much if the list above looks long ;-)


Regards,

Christian Boltz

[1] I'm doing openSUSE betatests since some years, and reported more
    than 1000 bugs in this time ;-)
-- 
dU hAsT nAtUeRlIcH rEcHt. MaN mUsS sIcH bEiM lEsEn NuR dArAn GeWoEhNeN.
mAcHt DaNn KeInEn UnTeRsChIeD mEhR.       [Andreas Kneib in suse-linux]