Problems found and future feature requests

8 views
Skip to first unread message

bensch128

unread,
Jun 3, 2008, 7:17:17 PM6/3/08
to reviewboard
Hi, I'd like to say that reviewboard is working great for me but I
have discovered some problems and missing features.
1) diff in post-review requires diff executable
I had to patch post-review to use diffutils instead of calling out to
"diff -urNp". On windows diff isn't part of the standard distribution
and I rather not force users to install it. Using diffutils as a drop
in replacement for the perforce client worked perfectly. I didn't test
with binaries though

2) ReviewBoard calls out to patch which only works under cygwin on
windows.
This isn't really a problem now on my test server but once we move it
to apache on windows, I wonder if this will become a problem

2) "View Diff" leads to a 404 if post hasn't been published yet.
This was extremely confusing because the user thought the review had
been published but maybe because of a user error, it wasn't clear to
me, the reviewer, that the review hadn't been properly published.
Unfortunately,
the user sent me the review, I clicked on it and then clicked on "View
Diffs" This lead to a 404 page instead (1) hidding the View diffs and
indicating that the review hadn't been published or (2) leading to a
page stating that the review hadn't been published yet. Once the user
properly published, then the diffs showed up correctly

3) Need support for either marking lines as defects/bugs or be able to
add severity levels to comments
Is there a standard way to do this now or is this another field that
needs to be added to the comments?

4) How can a user or admin mark that user as receiving all reviews?
One of our users wants to be able to see all reviews which are posted.
(He's the senior architect)
Is there a way to do this already?

5) Linking the django account with perforce
Currently, the users have to always authenticate with their
reviewboard username and password.
I'd like to have reviewboard use preforce to authenticate the user so
post-review doesn't need to always ask the user for the password.

6) Adding perforce triggers to prevent submission unless the review is
successful.
Has anyone tried doing this yet?

Thanks for the amazing work. I'll try to get approval to post my
patches to post-review eventually.

Cheers
Ben

bensch128

unread,
Jun 3, 2008, 7:44:59 PM6/3/08
to reviewboard
This is a followup with other issues I've found:
1) post-review needs options to manually set the scm to use.
Currently, it defaults to looping through a list until it finds one
which matches. Our users only use perforce so I want to force that all
of the time.
an option like --scm perforce --port p4port --client p4client --user
p4user would be helpful. Right now, i had to move PerforceClient() to
the beginning of the list.

More later when i can think of them... ;)
Cheers
Ben

Christian Hammond

unread,
Jun 3, 2008, 8:29:47 PM6/3/08
to revie...@googlegroups.com
This is a good list. I'll address them inline.


On Tue, Jun 3, 2008 at 4:17 PM, bensch128 <bens...@yahoo.com> wrote:

Hi, I'd like to say that reviewboard is working great for me but I
have discovered some problems and missing features.
1) diff in post-review requires diff executable
I had to patch post-review to use diffutils instead of calling out to
"diff -urNp". On windows diff isn't part of the standard distribution
and I rather not force users to install it. Using diffutils as a drop
in replacement for the perforce client worked perfectly. I didn't test
with binaries though

I'd love a patch for this.

 


2) ReviewBoard calls out to patch which only works under cygwin on
windows.
This isn't really a problem now on my test server but once we move it
to apache on windows, I wonder if this will become a problem

We have users with Windows servers hosting Review Board who have it working. I can't speak as to how painful it is though.

I can't begin to describe how painful it would be to rewrite patch for our use. patch is a very complicated piece of spaghetti code that works in a lot of situations with all kinds of diff formats, taking into account lots of quirks of the format. If we were to even try to provide our own implementation, Review Board likely wouldn't work for a lot of people for a long time, due to all the issues we would definitely hit.

patch is going to be required for a long time unless some great solution presents itself. Sorry :/

 
2) "View Diff" leads to a 404 if post hasn't been published yet.
This was extremely confusing because the user thought the review had
been published but maybe because of a user error, it wasn't clear to
me, the reviewer, that the review hadn't been properly published.
Unfortunately,
the user sent me the review, I clicked on it and then clicked on "View
Diffs" This lead to a 404 page instead (1) hidding the View diffs and
indicating that the review hadn't been published or (2) leading to a
page stating that the review hadn't been published yet. Once the user
properly published, then the diffs showed up correctly

Yeah. It would be nice to do something better here.


3) Need support for either marking lines as defects/bugs or be able to
add severity levels to comments
Is there a standard way to do this now or is this another field that
needs to be added to the comments?

I'm not sure how much policy and stuff I want to put in Review Board for this. It's a nice idea but it's a slippery slope. We could add those features and then someone else would want a variation on that, and then other people would want each comment to have someone sign off on it, or have bugs assign to certain lines, or whatever. It could easily get complicated and messy fast.

The way we choose to handle policy with Review Board is to just leave it as a social thing in the company. What I'd really recommend is just writing the information in the text field. Make it a standard to list that information.

In the future, when extensions are in, maybe someone can write an extension to provide additional data with comments. I don't think it belongs in the core, though.
 


4) How can a user or admin mark that user as receiving all reviews?
One of our users wants to be able to see all reviews which are posted.
(He's the senior architect)
Is there a way to do this already?

There's a Default Reviewers model that takes a regex and a user or group. You can have a regex of "." to match everything and have him as a default reviewer.

If you make use of groups, he can just join every group instead.

 
5) Linking the django account with perforce
Currently, the users have to always authenticate with their
reviewboard username and password.
I'd like to have reviewboard use preforce to authenticate the user so
post-review doesn't need to always ask the user for the password.

Someone would need to write a Django auth backend for this. There's no support for it today. Still, though, I imagine they'll have to log in anyway, since that's just how auth in Django tends to work. What you'd get is standard users/passwords without having to create new accounts for each person.

post-review shouldn't have to ask for a user/password that often, as long as the cookies are being saved.


6) Adding perforce triggers to prevent submission unless the review is
successful.
Has anyone tried doing this yet?

Not to my knowledge yet. I'd like to see something for this.
 

Thanks for the amazing work. I'll try to get approval to post my
patches to post-review eventually.

Cool :)



--
Christian Hammond - chi...@chipx86.com
VMware, Inc.

Christian Hammond

unread,
Jun 3, 2008, 8:30:22 PM6/3/08
to revie...@googlegroups.com
It is doing more work by checking each type, but it shouldn't pose any problem. What issues are you seeing with this?

Christian

bensch128

unread,
Jun 3, 2008, 11:02:19 PM6/3/08
to reviewboard


>
> I'd love a patch for this.
>

Sure. I posted it at http://rafb.net/p/RBm01q54.html
It's aimed at working with p4v and p4win easily but can be used with
other SCMs easily.
There's no explicit setting of which scm to use because of the ugly
parsing of options after the scm is selected.
I had to reorder which Client is chosed first in the list. This
should be explicitly set or error IMHO.

>
>
> > 2) ReviewBoard calls out to patch which only works under cygwin on
> > windows.
> > This isn't really a problem now on my test server but once we move it
> > to apache on windows, I wonder if this will become a problem
>
> We have users with Windows servers hosting Review Board who have it working.
> I can't speak as to how painful it is though.
>
> I can't begin to describe how painful it would be to rewrite patch for our
> use. patch is a very complicated piece of spaghetti code that works in a lot
> of situations with all kinds of diff formats, taking into account lots of
> quirks of the format. If we were to even try to provide our own
> implementation, Review Board likely wouldn't work for a lot of people for a
> long time, due to all the issues we would definitely hit.

Maybe reviewboard should just assume/validate that all uploads are in
unified format.
Then writing a python patcher shouldn't be too difficult.

>
> patch is going to be required for a long time unless some great solution
> presents itself. Sorry :/
>

>
> 3) Need support for either marking lines as defects/bugs or be able to
>
> > add severity levels to comments
> > Is there a standard way to do this now or is this another field that
> > needs to be added to the comments?
>
> I'm not sure how much policy and stuff I want to put in Review Board for
> this. It's a nice idea but it's a slippery slope. We could add those
> features and then someone else would want a variation on that, and then
> other people would want each comment to have someone sign off on it, or have
> bugs assign to certain lines, or whatever. It could easily get complicated
> and messy fast.
>
> The way we choose to handle policy with Review Board is to just leave it as
> a social thing in the company. What I'd really recommend is just writing the
> information in the text field. Make it a standard to list that information.
>
> In the future, when extensions are in, maybe someone can write an extension
> to provide additional data with comments. I don't think it belongs in the
> core, though.
>

Alright. But I'm kinda more interested in the immediate solution ;)

>
>
> > 4) How can a user or admin mark that user as receiving all reviews?
> > One of our users wants to be able to see all reviews which are posted.
> > (He's the senior architect)
> > Is there a way to do this already?
>
> There's a Default Reviewers model that takes a regex and a user or group.
> You can have a regex of "." to match everything and have him as a default
> reviewer.
>
> If you make use of groups, he can just join every group instead.
>
> > 5) Linking the django account with perforce
> > Currently, the users have to always authenticate with their
> > reviewboard username and password.
> > I'd like to have reviewboard use preforce to authenticate the user so
> > post-review doesn't need to always ask the user for the password.
>
> Someone would need to write a Django auth backend for this. There's no
> support for it today. Still, though, I imagine they'll have to log in
> anyway, since that's just how auth in Django tends to work. What you'd get
> is standard users/passwords without having to create new accounts for each
> person.
>
> post-review shouldn't have to ask for a user/password that often, as long as
> the cookies are being saved.

Ug, that sucks. I was hoping someone had done it already...

>

cheers
Ben

bensch128

unread,
Jun 3, 2008, 11:14:07 PM6/3/08
to reviewboard
Oh and post-review should be packages with a standalone version of
simplejson.
I had to take the version of simplejson in reviewboard and
rewrite the import statements before telling the users to drop it into
their PYTHON_PATH.

It was not fun.

On Jun 3, 8:02 pm, bensch128 <bensch...@yahoo.com> wrote:
> > I'd love a patch for this.
>
> Sure. I posted it athttp://rafb.net/p/RBm01q54.html

Christian Hammond

unread,
Jun 3, 2008, 11:24:56 PM6/3/08
to revie...@googlegroups.com
You should be using the actual simplejson. Django has a modified one.

On most systems, you get install it using: easy_install simplejson

Christian

Leo Soto M.

unread,
Jun 5, 2008, 11:57:53 AM6/5/08
to revie...@googlegroups.com
On Tue, Jun 3, 2008 at 8:30 PM, Christian Hammond <chi...@chipx86.com> wrote:
> On Tue, Jun 3, 2008 at 4:44 PM, bensch128 <bens...@yahoo.com> wrote:
> >
> > This is a followup with other issues I've found:
> > 1) post-review needs options to manually set the scm to use.
> > Currently, it defaults to looping through a list until it finds one
> > which matches. Our users only use perforce so I want to force that all
> > of the time.
> > an option like --scm perforce --port p4port --client p4client --user
> > p4user would be helpful. Right now, i had to move PerforceClient() to
> > the beginning of the list.
>
> It is doing more work by checking each type, but it shouldn't pose any
> problem. What issues are you seeing with this?

Speaking from my own experience with ReviewBoard, it doesn't always
work well when the working directory is being managed by multiple
SCMs.
--
Leo Soto M.
http://blog.leosoto.com

Reply all
Reply to author
Forward
0 new messages