Re: Removed lines in responsible

[Lars Henriksen]

Hello,

PR gnatsweb/341, submitted earlier this year, is concerned with gnatsweb's
handling of the responsible field. Others have been puzzled as well (see
PR gnatsweb/195).

On Mon, Oct 14, 2002 at 06:44:35PM +0200, Dieperink Alwin wrote:

> In our project, people are coming and leaving. For each change, we adapt the
> "responsible" file. When someone is leaving, then we reassign each open PR
> to someone else and then remove that person from the file. But we don't
> change all the closed PRs (this is a lot of work).

This is also what we do and, I suspect, many others. It is really a general
aspect of GNATS 4 enumeration types which can be changed at will: you can
safely add new enumeration values, but if you remove one, you in a way
invalidate fields with that value; but the value is kept in the index file as
well as in the PRs. This in itself need not be a problem. If you view an
entire PR, GNATS just displays the contents, invalid field values and all.
It becomes more subtle with formatted queries.

>                                                    Now the behaviour is
> somewhat strange (or at lease inconsistent).
>
> - The responsible for PR 123 is John (I looked in the PR file). The index
> file also contains John. John isn't anymore in the responsible file.

To understand what is going on, it is necessary to distinguish between GNATS
and gnatsweb. First GNATS. I'll use your own example and in addition assume
that Jack is responsible for PR 456 and is the last of 13 entries in the
responsible file.

Look at the following query commands:

1$ query-pr --format '"%s %s" number responsible' 123
2$ 123 John
3$ query-pr --format '"%s %s" number responsible' 456
4$ 456 Jack

If the format string is changed to return the responsible as an integer,
the results are:

5$ query-pr --format '"%s %d" number responsible' 123
6$ 123 0
7$ query-pr --format '"%s %d" number responsible' 456
8$ 456 13

The integer returned is the position (index) of the value in the enumeration
type, beginning with 1. So 13 means that Jack is the 13th entry in the
responsible file and 0 (zero) that John is not in the file.

I think this is sensible behaviour by GNATS. But of course it must be supported
properly by GNATS clients like gnatsweb, and this is where the difficulties
begin. When gnatsweb performs queries, it uses the integer format identifier
%d and does the translation back to strings itself. The culprit is in
display_query_results():

      # The query returned the enums as numeric values, now we have to
      # map them back into strings.
      if ($fieldtypes[$whichfield] eq 'enum')
      {
        my $enumvals = fieldinfo($columns[$whichfield], 'values');
        $fieldcontents = $$enumvals[$fieldcontents - 1] || 'invalid';
      }

If $fieldcontents is 0, $$enumvals[$fieldcontents -1] is the last enumeration
value.

> - When doing a query, the shown responsible is Jack. In fact, Jack is the
> last one in the responsible file. When I add another at the end of file,
> it's that new one which is displayed.

This is the bug explained above.

> - When viewing the PR, the shown responsible is John.

This is OK in my opinion, even though John is no longer a possible value.

> - When editing the PR, then responsible now becomes gnats-admin, as it is
> the first one in the list.

Yes. Gnatsweb tries to display the responsible person as the default value
in the pop-up list, but can't and selects the first entry instead.

> What should be the correct behaviour of Gnats ? How should we handle leaving
> person ? We don't want to change all the closed PRs (several hundreds) and
> don't want to keep in the responsible file all the persons who have once
> been responsible for a PR.

As I said above, I think GNATS is OK. The problem lies with gnatsweb, and I
am not sure what the correct solution is.

A quick fix for the responsible file that does not require any code changes,
is to insert two identical entries, one at the beginning and one at the end:

unknown:Name is not in the "responsible" file.:gnats-admin

This will at least alert users that something is wrong.

The query format could be changed to use %s instead of %d. I don't believe
there is any performance penalty, maybe even an improvement? That still leaves
the Edit Page.

Another possibility is to introduce a value to display if the original value
for some reason cannot be displayed. A candidate is "unknown" which gnatsweb
already uses and disallows for a few fields in a new PR.

I have a fix that may or may not suit other installations.

The zero return value should be tested for:

  $fieldcontents =
    $fieldcontents ? $$enumvals[$fieldcontents - 1] || 'invalid' : 'unknown';

This maps zero to "unknown" which is then displayed in query results.
(The check for an invalid index value is a bit paranoic and could be removed.)

This takes care of the first problem above. assuming that no enumeration
type has "unknown" as a valid value, or that "unknown" is valid (i.e. present
in the file), but disallowed in other ways.

The third problem (edit PR) has been fixed by adding a blank entry first in the
responsible file:

# Blank pseudo user. 
# Displayed by gnatsweb in "Edit Problem Report" when the PR responsible is
# missing in this file.
:The name is no longer valid. Change it or use "view" to see it. -:

Gnatsweb already catches a blank responsible field in submitedit(), but the
context is a bit strange:

    if($db_prefs{'user'} eq "" || $fields{$RESPONSIBLE_FIELD} eq "")
    {
# dtb this appears to make it impossible to edit a PR with a blank
# Responsible field.  this might not be the right thing to do...
      error_page("Responsible party is '$fields{$RESPONSIBLE_FIELD}', user is 
'$db_prefs{'user'}'");
      last LOCKED;
    }

I have simply deleted the user part:

    if ($fields{$RESPONSIBLE_FIELD} eq "") {
      error_page("$RESPONSIBLE_FIELD is blank");
      last LOCKED;
    }

Whew! That was a long story and a bit messy. I made the fixes a while back
and I am not entirely happy with them now that I review them. The last part,
particularly, is a temporary fix. Only the responsible field is handled and
that by changing the file.