If the summary is more than 300 chars Trim it down

Submitter:	JSlominski	Reviewers
Branch:		Groups:	reviewboard
Bugs:	515	People:
Change Number:	None	Repository:	Review Board SVN

Description:

When using post-review with perforce there is an error if the CL description has more that 300 chars without a newline. A SQL warning is generated (truncating summary) that will cause post0review to throw an error 500. There is an example of the error page in Bug 515. This is my first attempt in Python so the code may not be the best. I created a function that will truncate a string at the first '.' or after the 300th char.

Testing Done:

I submitted and updated a review that had more than 300 chars without a \n and a '.'.
I submitted and updated a review that had more than 300 chars without a \n and did not have a '.' 
I submitted and updated a review that had more than 300 chars without a \n and had '.' after the 300th char

Loading diff fragment...

I for got to take out my debut messages. I will remove them in the morning

I started to review this but then I began to think about what we want to do here. I'm not sure truncating like this is really the desired behavior. We should be limiting the size in the web UI (which I believe we do) and we should probably provide a validation error to clients using the web API that set a value that's too long. It's then up to the calling program to limit the size.

So the truncation logic should probably go in post-review.

David Trowbridge 5 months, 1 week ago (August 1st, 2008, 2:26 a.m.)

We'd need to handle this in our perforce changeset description parser, too (bug 438)

JSlominski 5 months, 1 week ago (August 3rd, 2008, 2:32 p.m.)

I made the requested changed.  I tried doing this in post-review first but it would still add the full description to it.  I will look at it again today and see if I can fix this in post-review

Loading diff fragment...

This would be "truncate".

Loading diff fragment...

All this file stuff should go away.

Loading diff fragment...

Trailing whitespace. Also in other places in the change.

Loading diff fragment...

This algorithm isn't quite right. Two problems with it:
1. If the first sentence is longer than 300 characters, it'll still leave a string too long.
2. It would be nice to include as many sentences as possible within the allowed 300 characters instead of just the first.

JSlominski 4 months, 2 weeks ago (August 22nd, 2008, 8:17 a.m.)
```
Please see the updated diff
```

Loading diff fragment...

One space after ,s

Loading diff fragment...

One space after ,s

Loading diff fragment...

This is way more complicated than it needs to be.  Here's what I suggest:

def truncate(string, n):
    if len(string) > n:
        string = string[0:n]
        i = string.rfind('.')
        if i != -1:
            string = string[0:i + 1]
    return string

JSlominski 4 months, 1 week ago (August 27th, 2008, 6:09 a.m.)

Made these changes.  I had no idea that python could be so simple.

Loading diff fragment...

Is is possible to pull the number 300 from a config file? Would be nice to avoid magic numbers in the code... just my $0.02

Christian Hammond 4 months, 2 weeks ago (August 26th, 2008, 1:12 p.m.)

I don't feel this is something worth customizing, but it should be a constant in the file.

David Trowbridge 4 months, 2 weeks ago (August 26th, 2008, 1:17 p.m.)

300 is the limit of the field in the model, not some value we picked out of a hat.

A comment to this effect probably wouldn't hurt.

JSlominski 4 months, 1 week ago (August 27th, 2008, 6:10 a.m.)
```
Added the comment and global variable
```

Loading diff fragment...

No doubt, obviously not trying to start a flame-war or nit-pick session. Commenting is a great idea, especially if you factor out the 300 into a global or class variable like "max_summary_length":

/* truncate summary to avoid p4 error - experimental limit is 300 chars */
self.summary = truncate(self.summary, max_summary_length)

Committed as r1459.  Thanks!

Review Board

beta

This change has been marked as submitted.