Bug 444778 - FEAT: HwCert catalog will retire and stop accepting rhr2 results
FEAT: HwCert catalog will retire and stop accepting rhr2 results
Product: Red Hat Ready Certification Tests
Classification: Retired
Component: web site (Show other bugs)
All Linux
urgent Severity medium
: ---
: ---
Assigned To: XINSUN
Yu Shao
Depends On:
  Show dependency treegraph
Reported: 2008-04-30 11:09 EDT by XINSUN
Modified: 2008-07-08 04:25 EDT (History)
5 users (show)

See Also:
Fixed In Version: 2008/06/19 push
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-07-08 04:25:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)
Have tested, Pls review. (4.72 KB, patch)
2008-05-14 05:13 EDT, XINSUN
no flags Details | Diff
Improve patch: (7.16 KB, patch)
2008-05-16 16:13 EDT, XINSUN
no flags Details | Diff
Patch Improve: (7.25 KB, patch)
2008-05-29 07:14 EDT, XINSUN
no flags Details | Diff
Patch Improve: (7.74 KB, patch)
2008-05-30 03:20 EDT, XINSUN
no flags Details | Diff
Patch Improve (8.37 KB, patch)
2008-06-23 04:03 EDT, XINSUN
no flags Details | Diff

  None (edit)
Description XINSUN 2008-04-30 11:09:18 EDT
HwCert catalog will retire and stop accepting rhr2 results.
Comment 1 XINSUN 2008-05-14 05:13:49 EDT
Created attachment 305340 [details]
Have tested, Pls review.
Comment 2 Rob Landry 2008-05-16 11:21:02 EDT
Close, for now we should allow a hwcert-edit permissions to override and allow
the catalog to take an rhr2 rpm but only from show.cgi via attachment.cgi; also
we'll want to work on the wording of the error message to be more friendly.

An example for the error message...

'$filename' appears to be of type <i>rhr2 results package</i>.  rhr2 has been
obsoleted by hts.   Please retest using hts and resubmit the results package.
Comment 3 XINSUN 2008-05-16 16:13:44 EDT
Created attachment 305742 [details]
Improve patch:

1. Better error message/text.
2. Add the checkbox "AllowRHR2" to ensure the hwcert_edit can allow/override
the rhr2 package.
Comment 4 Rob Landry 2008-05-16 16:33:33 EDT
I'm not a fan of the use of $comment in patch 305742. $comment contains an error
message string, it would be better if we could use the "obsolete_rhr2_package"
case or similar such that we can keep the html error printing in one location
instead of mixing text in with the code.

The attach.cgi looks to break the catalog's ability to take rpm's as an
attachment.  I think better would be a warning in the event text that the rpm
does not appear to be an hts results rpm so it will be treated as a regular
attachment.  This would also means that the "hts_package_required" error case is
not required.

Last up; it seems like we should not need to do this is it an rhr2 or hts rpm in
so many place.  I could be wrong, but I thought that much of the attachment code
had been flatted during the transition to using bz attachements?  such that
there would only be two places for this code which align to the create vs.
append workflows in the catalog.
Comment 7 XINSUN 2008-05-29 07:14:11 EDT
Created attachment 307030 [details]
Patch Improve:

(a) Have an option for @redhat.com override--"allow RHR2 package" checkbox.
(b) For other users figure out they are rhr2 package, reject them but offer the
optionto treat it as a regular attachment instead --"Attach as a common file".
checkbox. it can be auto-checked when the attachment's filename is *.rpm and
contains /rhr2/ which is implemented by javascript. 
(c) To "hwcert_edit" people who can see those two checkboxes,those two
checkboxes are mutual exclusion for checking.

The patch has been deployed in ~xisun5/ on bugdev and test well.


Pls review.

Best Regards!
Comment 8 Rob Landry 2008-05-29 10:35:16 EDT
hwcert-edit membership needs to be enforced in the cgi as well, technically it
looks like a user could manually add to the url line which would allow them to
attach rhr2.

Can we avoid the 0 vs. 1 in the CGI and use true vs. false instead?  It's
cleaner when reviewing the code later to understand what's going on to see foo
is true instead of foo is 1; also depending on the language 1 vs. 0 as compared
to true vs. false changes.

We should probably bold or some other highlight to explain why the attach as
file checkbox has been automatically checked, hopefully this will help users. 
Probably the best thing is a little box with blue text next to the checkbox
should appear which says the filename match rhr2 which is no longer accepted,
automatically attaching as file" or something similar.  You'll have to play
around and see what looks good and makes sense from a user point of view.

Can we replace "Attach AS A Common File." with  "Attach as file." or does that
look confusing for users?

Change "Allow RHR2 Results Package." to "One Time Override: Allow rhr2 results."?

Change "You must upload the" to "You must update an"

Drop the " and resubmit the results package" from the error message.
Comment 9 XINSUN 2008-05-30 03:20:29 EDT
Created attachment 307171 [details]
Patch Improve:

Rob, Thanks a lot for giving me kindly suggestions on that patch.
I have improved them as your mention.
a)limit hwcert-edit membership in the cgi
b)change 0 vs 1 to  false vs true in the cgi
c)Add a blue text "The filename match rhr2 which is no longer accepted,
automatically attaching as file" below the "attach as file" checkbox. (maybe
below the checkbox is better next to the checkbox.) it appears	when the attach
as file checkbox has been automatically checked (upload rhr2 ).
d)Correct to "Attach as file."
e)Change "Allow RHR2 Results Package." to "One Time Override: Allow rhr2
f)Change "You must upload the" to "You must update a".Drop the " and resubmit
the results package" from the error message.

pls review. :)

Best Regards!
Comment 10 Rob Landry 2008-06-16 22:12:07 EDT
In testing on -xisun5 the "Allow rhr2 results." did not automatically check when
an rhr2 filename was typed in; however after manually checking the box and then
changing the filename to a -hts the checkbox did automatically uncheck so we
need to check the logic there.
Comment 11 Rob Landry 2008-06-16 23:33:42 EDT
Patch comments:

@@ -1020,10 +1021,21 @@

The logic is confusing; it's a weird combination of negative and positive test
conditions with nested IFs which looks like it would work but I'd like to see if
we can see if there is a way that makes the logic more obvious as to what's
trying to be accomplished.

@@ -957,9 +957,13 @@

Perhaps processResultsRpm could also include a comment to indicate that the
override was selected.

@@ -30,6 +46,24 @@

Probably would be better if there wasn't two check-boxes as it would make no
sense to allow for both attach as file and override to allow rhr2.  Changing
this to a single checkbox might be a cleaner UI and might provide for clearer
logic in the 1st section and help with the messaging from processResultsRpm in
the second section.
@@ -30,6 +46,24 @@

Something isn't quite right with the blue highlighted text presentation; might
be cleaned up as part of the single checkbox approach?  It does work as
originally designed but doesn't seem quite right from the usage.

@@ -647,6 +651,10 @@

For the type of file it would be best if we could report the "provides"; meaning
something like...

 file $foo appears to provide $rpmprovides which has been deprecated.  HTS
obsoleted rhr2 as of June 19, 2008 and is no longer accepted.  Retesting using
hts will be required.  Please contact support if you have any questions.

...or something similar.
Comment 12 XINSUN 2008-06-23 04:03:02 EDT
Created attachment 310015 [details]
Patch Improve

This patch has been checked into cvs.

Note You need to log in before you can comment on or make changes to this bug.