This is my feedback on the code that came with the email I am replying to.
User interface:
Having all the buttons below a line on the OpenVPN front page is good. However, the button to manage the nets should say "Static IP address pools". Nothing else.
In the box below, the rearrangement of the buttons broke the form on which you can upload a CA certificate. You cannot really make the link to the upload button.
Why is it necessary to stop the roadwarrior server to manage the IP address pools? I guess a note that all changes require a restart would be sufficient.
Client table:
I think showing the network a client is in is useful, but it does not leave much space for the remark. Maybe we could drop the "Common name" column. The legend below the table it somewhat messy and unaligned. Don't know if that was already the case before.
Static IP address pool configuration page:
Language problems: There are not "nets", just "networks". It is "IP address", neither "ip address" nor "IP". "Getting an IP address" is an "IP address assignment". Title says "add net".
Table: "Net name" -> "Name", "IP address pool" -> "Network", "possible Adresses" -> "Usable Addresses" or "Assignable Addresses". I would also still prefer showing the usage as in "1/32". Align the subnet addresses to the center. The table misses a headline.
When editing a network, there is no "cancel" button and the submit button says "change net" which does not fit very well. You cannot edit the network address AT ALL, just the name. Let it just say "Save".
The name field accepts values with a comma. Try it out to see the damage. It is also possible to add subnets twice or to add overlapping subnets.
Client configuration page:
There is again "net" all over the page. Again "Net name" -> "Name", "IP address pool" -> "Network", "Host address" -> "Assigned IP address". The table is missing a headline.
"Dynamic OpenVPN server addresspool" -> "Dynamic OpenVPN IP address pool". One could also add the network, just to be sure.
"Net to route" does not really explain what those boxes to. "Behind Client" is not a possible explanation because it depends on the point of view.
The select box below is really cool. However, "Green" is spelled mixed case, the other options "BLUE" and "ORANGE" are all upper case. Why is that?
The DNS server boxes say "END" which is a programming error.
When creating a new roadwarrior connection, I would prefer to have the authentication stuff above the "Advanced client options". However, the authentication part is a real box and the other stuff is not. Please consider making this consistent as well.
Code review:
I see that the entire web user interface is a mess and I see that nobody likes to touch it. But we need to make sure that we don't introduce new problems, because IPFire 2 needs to be maintained for many years and I don't want to make this a hell to ourselves.
So. Don't comment in German. Comments in English are a requirement. In case a function replaces an other one, remove the old (and buggy?) function. Make sure that it is clear what arguments a function expects and what the return value is. Use meaningful names for variables. I see that "$a, $b, $c, $d" is much more easy to type, but I find "$byte1, $byte2, ..." better. Also code in a way that is easy to understand. Perl let's users to write many things in one line, but that code does not execute faster. Make clear what you do.
Remove the tags which say which lines have been added for the CCD functionality. We use git to track those things.
Don't re-indent functions that already exist.
Why is there getnextip and NextIP2?
I guess that's enough for now.
Michael
P.S. Please don't open another email thread because my inbox is stuffed with emails that apparently are not connected to each other. It is
On Thu, 2012-11-08 at 16:19 +0100, Alexander Marx wrote:
Dear List!
Just talked to Michael. He would like to see the standard networks in the routingbox (Green,Orange and Blue if they exist).
So i coded it!
I deleted the standard route line in the server config, and therefore i put GREEN in the routingbox as Default.
As always, comments are welcome.
Alexander Marx
Fachinformatiker Systemintegration
Development mailing list Development@lists.ipfire.org http://lists.ipfire.org/mailman/listinfo/development