Summary

Specify the URL for users when recording ChangeDescription entries.

Review Request #14410 — Created April 29, 2025 and submitted May 4, 2025, 3:39 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When recording ChangeDescription entries for a change in ownership on

a review request, we rely on User.get_absolute_url(). This is defined

globally for users via settings.ABSOLUTE_URL_OVERRIDES, but the value

doesn't take into account Local Sites, and on RBCommons we don't have

this set at all, meaning this function is not available on User.
To address this properly, ChangeDescription.record_field_chnage()

now takes an optional build_url_func(), which can be used to let a

caller handle URL computation for the objects. This is specified by

OwnerField to compute a URL relative to the Local Site or global site

for the parent review request.

Testing Done

Removed the ABSOLUTE_URL_OVERRIDES and reproduced the original problem
publishing. Verified that this patch fixed that, and generated a correct
URL for both the global site and Local Site.

Tested this on RBCommons as well.

Commits

Summary	ID
Specify the URL for users when recording ChangeDescription entries. When recording `ChangeDescription` entries for a change in ownership on a review request, we rely on `User.get_absolute_url()`. This is defined globally for users via `settings.ABSOLUTE_URL_OVERRIDES`, but the value doesn't take into account Local Sites, and on RBCommons we don't have this set at all, meaning this function is not available on `User`. To address this properly, `ChangeDescription.record_field_chnage()` now takes an optional `build_url_func()`, which can be used to let a caller handle URL computation for the objects. This is specified by `OwnerField` to compute a URL relative to the Local Site or global site for the parent review request.	fba37c6f733e3a86e80495ecbe24734c69d0213d

Summary

Specify the URL for users when recording ChangeDescription entries.

When recording `ChangeDescription` entries for a change in ownership on a review request, we rely on `User.get_absolute_url()`. This is defined globally for users via `settings.ABSOLUTE_URL_OVERRIDES`, but the value doesn't take into account Local Sites, and on RBCommons we don't have this set at all, meaning this function is not available on `User`. To address this properly, `ChangeDescription.record_field_chnage()` now takes an optional `build_url_func()`, which can be used to let a caller handle URL computation for the objects. This is specified by `OwnerField` to compute a URL relative to the Local Site or global site for the parent review request.

fba37c6f733e3a86e80495ecbe24734c69d0213d

Issues

Description	From	Last Updated
Can we add a unit test that uses build_url_func?	david	April 30, 2025, 1:10 a.m.
From the implementation it looks like these should be _T \| str \| None	david	April 30, 2025, 1:18 a.m.
Don't feel like you need to do it, but it might be nice to have @overrides to indicate this?	david	April 30, 2025, 1:18 a.m.
Can we add typing for this and the other inner methods?	david	April 30, 2025, 1:18 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can we add a unit test that uses build_url_func?

reviewboard/changedescs/models.py (Diff revision 1)

The issue has been dropped. Show all issues

From the implementation it looks like these should be _T | str | None

chipx86 April 30, 2025, 1:21 a.m.

_T is roughly Any so it covers str and None, but a list[...] as a value causes problems for the _T in build_uri_func (which should be an item). Because I have a lot of work toward a modernization of the whole ChangeDescription work with enhanced typing, I'm going to keep this very simple.

reviewboard/changedescs/models.py (Diff revision 1)

The issue has been dropped. Show all issues

Don't feel like you need to do it, but it might be nice to have @overrides to indicate this?

david April 29, 2025, 8:02 a.m.
```
Sorry, meant @overload
```

chipx86 April 29, 2025, 3:24 p.m.

I have a larger change that's been sitting in my tree to make a lot of the typing of ChangeDescription nicer. I'm trying to avoid duplicating any of that work at the moment, so much of this change is just bare minimum typing.

reviewboard/changedescs/models.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Can we add typing for this and the other inner methods?
```

Change Summary:


Switched build_url_func's type for the item to be Any, since _T provided the wrong type when storing lists, and added overloads to cover that case.
Added a unit test for build_url_func.

Commits:

	Summary	ID
	Specify the URL for users when recording ChangeDescription entries. When recording `ChangeDescription` entries for a change in ownership on a review request, we rely on `User.get_absolute_url()`. This is defined globally for users via `settings.ABSOLUTE_URL_OVERRIDES`, but the value doesn't take into account Local Sites, and on RBCommons we don't have this set at all, meaning this function is not available on `User`. To address this properly, `ChangeDescription.record_field_chnage()` now takes an optional `build_url_func()`, which can be used to let a caller handle URL computation for the objects. This is specified by `OwnerField` to compute a URL relative to the Local Site or global site for the parent review request.	e97119d48208db0297315b48f04002e6c93eb2fd
	Specify the URL for users when recording ChangeDescription entries. When recording `ChangeDescription` entries for a change in ownership on a review request, we rely on `User.get_absolute_url()`. This is defined globally for users via `settings.ABSOLUTE_URL_OVERRIDES`, but the value doesn't take into account Local Sites, and on RBCommons we don't have this set at all, meaning this function is not available on `User`. To address this properly, `ChangeDescription.record_field_chnage()` now takes an optional `build_url_func()`, which can be used to let a caller handle URL computation for the objects. This is specified by `OwnerField` to compute a URL relative to the Local Site or global site for the parent review request.	31684619ee118721f9a16603ac753b291ee10b3e

Diff:

Revision 2 (+282 -30)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/changedescs/tests.py
	reviewboard/reviews/builtin_fields.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Added a missing version in a Version Added.

Commits:

	Summary	ID
	Specify the URL for users when recording ChangeDescription entries. When recording `ChangeDescription` entries for a change in ownership on a review request, we rely on `User.get_absolute_url()`. This is defined globally for users via `settings.ABSOLUTE_URL_OVERRIDES`, but the value doesn't take into account Local Sites, and on RBCommons we don't have this set at all, meaning this function is not available on `User`. To address this properly, `ChangeDescription.record_field_chnage()` now takes an optional `build_url_func()`, which can be used to let a caller handle URL computation for the objects. This is specified by `OwnerField` to compute a URL relative to the Local Site or global site for the parent review request.	31684619ee118721f9a16603ac753b291ee10b3e
	Specify the URL for users when recording ChangeDescription entries. When recording `ChangeDescription` entries for a change in ownership on a review request, we rely on `User.get_absolute_url()`. This is defined globally for users via `settings.ABSOLUTE_URL_OVERRIDES`, but the value doesn't take into account Local Sites, and on RBCommons we don't have this set at all, meaning this function is not available on `User`. To address this properly, `ChangeDescription.record_field_chnage()` now takes an optional `build_url_func()`, which can be used to let a caller handle URL computation for the objects. This is specified by `OwnerField` to compute a URL relative to the Local Site or global site for the parent review request.	fba37c6f733e3a86e80495ecbe24734c69d0213d

Diff:

Revision 3 (+284 -30)

Show changes

	reviewboard/changedescs/models.py
	reviewboard/changedescs/tests.py
	reviewboard/reviews/builtin_fields.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.1.x (bc2ec7d)