Re: CCD 8.11.2012

Re: CCD 8.11.2012

[Michael Tremer]

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

On Thu, 2012-11-15 at 13:17 +0100, Alexander Marx wrote:
> 
> I will answer the original here:
>   
> 
> 
> 
> Am 13.11.2012 16:08, schrieb Michael Tremer:

> > 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.
> The Button is clickable when server is stopped. That makes sense
> because when you can edit the static networks, they will be written in
> the server.conf and so the ovpn server needs a restart. 
> Other Question: WHY AM I NOT ABLE TO EDIT ADVANCED SERVER OPTIONS,
> WHEN SERVER IS RUNNING?!!!

Right, that is exactly the same problem here. I know that you cannot
edit the settings, but the user should still be able to view them.

> > The legend below the table it somewhat messy and unaligned. Don't know
> > if that was already the case before.
> Was the case before, didn't touch that!

Okay. Using the German translation of the WUI does not show this error.

> > 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.
> Fixed.

Well, I cannot have a space in the description, which is not cool.

> > 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.
> Fixed, except Headline. Don#t know if that makes sense here. I can fix
> that, too.

I think so. But it's not a must for me. Let it be as it is right now,
then.

> > "Dynamic OpenVPN server addresspool" -> "Dynamic OpenVPN IP address
> > pool". One could also add the network, just to be sure.
> Fixed.

Could you please add the network to that line?

> > "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.
> We phoned on that one and agreed to name it "Network behind client"
> And "Network behind IPFIRE".
> If you don't like it now, tell me how to name it!

It's not fine. Indeed it is hard to find a better label for those boxes,
but these are bogus.

> > 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?
> Fixed. Was in the language file en.pl green was "Green" orange "ORANGE
> and blue BLUE

That's bad.

> > 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.
> Fixed.

Didn't check that.

> >  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.
> Also phoned on that one and agreed to kick the last comments when code
> gets mainstream.

Yeah, that was today :D

> SO: WHICH HALF OF THE MAIL DID I MISS?!

See above.

Re: CCD 8.11.2012

[Michael Tremer]

[-- 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