Re: CCD 8.11.2012

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 4453 bytes --]

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(a)lists.ipfire.org
> http://lists.ipfire.org/mailman/listinfo/development