Wrong URL insert in database for review usr change
Review Request #3771 — Created Jan. 19, 2013 and discarded
Wrong URL insert in database for review usr change The wrong URL was inserted in database for user change in reviews (added or deleted), when the root is not / but a subfolder. The Django function that was used to retrieve the URL get_absolute_url() for a particular user was returning the wrong url. So we now have a new function that generates the right URL from the local_site_name and username.
Tested with a different SITE_ROOT parameter. (e.g: SITE_ROOT = '/reviewboard/' instead of SITE_ROOT = '/')
| Description | From | Last Updated |
|---|---|---|
|
Col: 80 E501 line too long (98 > 79 characters) |
 reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (87 > 79 characters) |
reviewbot
|
|
|
Col: 30 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 72 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 9 E301 expected 1 blank line, found 0 |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 35 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 80 E501 line too long (90 > 79 characters) |
reviewbot
|
|
|
Col: 67 W291 trailing whitespace |
reviewbot
|
|
|
Col: 30 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 30 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 50 W291 trailing whitespace |
reviewbot
|
|
|
Col: 21 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 68 W291 trailing whitespace |
reviewbot
|
|
|
Col: 29 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 25 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
According to PEP-8, imports should be broken up into three groups, each separated by a blank line (standard library, third-party … |
 david
|
|
|
I think this will break for any model that doesn't have a "username" field (for example, if the target_groups field … |
david
|
|
|
Col: 30 E128 continuation line under-indented for visual indent |
reviewbot
|
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/changedescs/models.py reviewboard/reviews/models.py Ignored Files: -
-
-
-
-
-
-
-
According to PEP-8, imports should be broken up into three groups, each separated by a blank line (standard library, third-party libraries, this package). From the "reviewboard" point of view, django/djblets are third-party, and reviewboard is "this package", so we should have a blank line between these.
-
I think this will break for any model that doesn't have a "username" field (for example, if the target_groups field changes). It's also kind of weird in that the code here is very generic, but you're adding specific knowledge of types that are elsewhere in the codebase (the idea is that "reviews" depends on "changedescs", not the other way around). I believe a better solution would be to pass in a callable instead of a boolean: def record_field_change(self, field, old_value, new_value, name_field=None, get_item_url_func=None): .... if name_field: results = [] for item in list(items): if get_item_url_func: url = get_item_url_func(item) else: url = item.get_absolute_url() results.append((getattr(item, name_field), get_item_url_func(item), item.id)) return results .... That way we can encode knowledge of the "user" type in reviews/models.py where we know we're building a set of the changed users.
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/changedescs/models.py reviewboard/reviews/models.py Ignored Files: