`BSTRelatedColumn` #1500

hepcat72 · 2025-04-05T14:14:07Z

Summary Change Description

Added BSTRelatedColumn and made field an instance member of BSTColumn.

Tweaked some of the BSTColumn doc string.
Checked for path being None in is_key_field in the model utilities.
Created a new warning class (DeveloperWarning) which I applied to all warnings under the bst_list_view category.

Affected Issues/Pull Requests

Resolves BSTRelatedColumn #1496
Merges into branch is_fk
Previous PR Integrated base class support for foreign keys #1499
Next PR BSTManyRelatedColumn #1501

Reviewer Notes/Requests

See comments in-line.

Checklist

This pull request will be merged once the following requirements are met. The
author and/or reviewers should uncheck any unmet requirements:

Review Requirements
- Minimum approvals: 1
- No changes requested
- All blocking issues resolved by reviewers
- Specific reviewers: @add_username_here
- Review period: 2 days
Associated Issue/PR Requirements:
- All issue requirements are satisfied
- All PR dependencies are merged
Basic Requirements
- Linters pass
- Tests pass
- Conflicts resolved
Overhead Requirements
- Tests implemented/updated
- Updated CHANGELOG.md Unreleased section

Also tweaked some of the BSTColumn doc string. Files checked in: DataRepo/models/utilities.py DataRepo/views/models/bst_list_view/column/field.py DataRepo/views/models/bst_list_view/column/related_field.py

Details: - Checked for path being None in is_key_field in the model utilities. - Fixed an errant reference to self.name in the create sorter/filterer methods. - Added the ability of selecting the only non-ID field from a model when selecting the display field. - Renamed display_field to display_field_path to disambiguate it from the 'field' instance attribute added to BSTColumn. - Fixed an issue where field_path was being set via kwargs instead of args. - Set a default of None for the display_field_path arg. Files checked in: DataRepo/models/utilities.py DataRepo/views/models/bst_list_view/column/related_field.py DataRepo/tests/views/models/bst_list_view/column/test_related_field.py

…warning class (DeveloperWarning) which I applied to all warnings under the bst_list_view category. Files checked in: DataRepo/tests/tracebase_test_case.py DataRepo/tests/views/models/bst_list_view/column/filterer/test_annotation.py DataRepo/tests/views/models/bst_list_view/column/filterer/test_field.py DataRepo/tests/views/models/bst_list_view/column/sorter/test_annotation.py DataRepo/tests/views/models/bst_list_view/column/sorter/test_field.py DataRepo/tests/views/models/bst_list_view/column/test_related_field.py DataRepo/utils/exceptions.py DataRepo/views/models/bst_list_view/column/annotation.py DataRepo/views/models/bst_list_view/column/filterer/base.py DataRepo/views/models/bst_list_view/column/related_field.py DataRepo/views/models/bst_list_view/column/sorter/base.py

hepcat72 · 2025-04-05T14:15:50Z

DataRepo/models/utilities.py

@@ -354,7 +354,6 @@ def field_path_to_model_path(


 def is_key_field(
-    model: Optional[Type[Model]],


C. Moved model to the second argument and gave it a default of None, since it is optional, which makes calling this method more simplistic if you already have a Field instance.

hepcat72 · 2025-04-05T14:18:02Z

DataRepo/models/utilities.py

+    if path is None:
+        raise ValueError("path must not be None")


C. I only recently realized that type hinting is not something that's enforced. So here, I check. I find this error more helpful than what you get without it. It doesn't reference the variable. It just says NoneType doesn't have attribute is_relation.

hepcat72 · 2025-04-05T14:19:08Z

DataRepo/tests/tracebase_test_case.py

-        def assertNotWarns(unexpected_warning=UserWarning):
+        def assertNotWarns(unexpected_warning=Warning):


C. Been learning more about Warnings. UserWarning is the default warning class. They all are derived from Warning, which is a subclass of Exception.

hepcat72 · 2025-04-05T14:20:25Z

DataRepo/tests/views/models/bst_list_view/column/filterer/test_annotation.py

@@ -2,6 +2,7 @@
 from django.test import override_settings

 from DataRepo.tests.tracebase_test_case import TracebaseTestCase
+from DataRepo.utils.exceptions import DeveloperWarning


C. I created a class named DeveloperWarning. The way I used it was to only issue these warnings in debug mode, so they won't go into the production log.

I edited a bunch of previously implemented tests to check for this instead of UserWarning. You can skip down to my next comment.

hepcat72 · 2025-04-05T14:24:12Z

DataRepo/tests/views/models/bst_list_view/column/test_related_field.py

+)
+
+
+@override_settings(DEBUG=True)


C. Review the tests if you want...

hepcat72 · 2025-04-05T14:25:23Z

DataRepo/views/models/bst_list_view/column/field.py

@@ -25,17 +25,15 @@ class BSTColumn(BSTBaseColumn):
        1. Annotations are not supported.  See BSTAnnotColumn.
        2. Related columns are only partially supported, as they will not be linked.  See BSTRelatedColumn.
        3. Many-related columns are not supported.  See BSTManyRelatedColumn.


C. Edited the docstring to move the stuff specific to the new BSTRelatedColumn class into the new class.

hepcat72 · 2025-04-05T14:27:32Z

DataRepo/views/models/bst_list_view/column/field.py

-        sorter = kwargs.get("sorter")
-        filterer = kwargs.get("filterer")


C. I realized that I didn't need to do this in the constructors of derived classes because the BSTBaseColumn calls the create_sorter and create_filterer methods that the derived classes define.

hepcat72 · 2025-04-05T14:28:37Z

DataRepo/views/models/bst_list_view/column/field.py

-                "filterer": filterer,
-            }
-        )
+        self.field = field_path_to_field(self.model, self.field_path)


C. I made a field instance member, because it's useful in derived classes and takes less code downstream.

hepcat72 · 2025-04-05T14:34:04Z

DataRepo/views/models/bst_list_view/column/field.py

-            }
-        )
+        self.field = field_path_to_field(self.model, self.field_path)
+        self.is_fk = is_key_field(self.field)


C. I realized that is_fk should be supported in the BSTColumn class, because a BSTRelatecColumn (and the eventual BSTManyRelatedColumn may or may not be used for foreign key fields, and this class can in fact process foreign key fields. It just doesn't support search/sort on them because the querysets end up rending the model objects in string context using the model's __str__ method. So the user sees something different than the value that would be applied to search/sort. The DB treated them as their literal pk value. So if a user were to say, sort by such a foreign key field, the sort will appear random, because they will see (for example) the model record's name, but ordered by its primary key.

The rest of the changes in this file are trivial, due to the new instance member being accessed via self.

hepcat72 · 2025-04-05T14:35:25Z

DataRepo/views/models/bst_list_view/column/filterer/base.py

@@ -10,6 +10,8 @@
 from django.utils.functional import classproperty
 from django.utils.safestring import mark_safe



C. More DeveloeprWarning changes you can skip...

hepcat72 · 2025-04-05T14:36:00Z

DataRepo/views/models/bst_list_view/column/related_field.py

@@ -0,0 +1,197 @@
+from __future__ import annotations


C. This is the crux of this PR.

hepcat72 · 2025-04-05T14:39:43Z

DataRepo/views/models/bst_list_view/column/related_field.py

+                f"display_field_path '{display_field_path}' is only allowed to differ from field_path '{field_path}' "
+                "when the field is a foreign key."
+            )
+


C. The conditional block below attempts to select a default display_field_path. The display_field_path is what makes search/sort possible when the field_path ends in a foreign key. Instead of rendering the foreign key as a model object in string context (which can potentially differ from any one single field in the model object), it renders the field value from the display_field_path, so that what the user sees is what search/sort will operate on.

If setting a default display_field_path fails, search and sort are disabled.

hepcat72 · 2025-04-05T14:41:50Z

DataRepo/views/models/bst_list_view/column/related_field.py

+                "searchable or sortable unless a display_field_path is supplied.",
+                DeveloperWarning,
+            )
+


C. The methods below are the same as in the base class, but the BSTSorter and BSTFilterer objects are created using the display_field_path instead of the field_path (when possible).

…xplain how a default is chosen. Files checked in: DataRepo/views/models/bst_list_view/column/related_field.py

Files checked in: DataRepo/views/models/bst_list_view/column/annotation.py DataRepo/views/models/bst_list_view/column/base.py DataRepo/views/models/bst_list_view/column/field.py DataRepo/views/models/bst_list_view/column/related_field.py

hepcat72 · 2025-04-06T19:21:43Z

DataRepo/tests/views/models/bst_list_view/column/test_related_field.py

+from DataRepo.views.models.bst_list_view.column.related_field import (
+    BSTRelatedColumn,
+)
+


C. These create some temporary test models...

hepcat72 · 2025-04-06T19:24:26Z

DataRepo/views/models/bst_list_view/column/annotation.py

@@ -128,22 +131,18 @@ def __init__(

        super().__init__(name, **kwargs)



C. Simplified these methods a little, added more precise type hinting, and used kwargs for more robust support for the constructors they call.

… into keyword arguments. Files checked in: DataRepo/views/models/bst_list_view/column/annotation.py DataRepo/views/models/bst_list_view/column/base.py DataRepo/views/models/bst_list_view/column/field.py DataRepo/views/models/bst_list_view/column/related_field.py

hepcat72 · 2025-04-06T19:54:16Z

DataRepo/views/models/bst_list_view/column/base.py

-    def create_sorter(self, sorter: Optional[str] = None) -> BSTBaseSorter:
+    def create_sorter(self, field=None, **kwargs) -> BSTBaseSorter:
        """Derived classes must define this method to set self.sorter"""
        pass

    @abstractmethod
-    def create_filterer(self, filterer: Optional[str] = None) -> BSTBaseFilterer:
+    def create_filterer(self, field=None, **kwargs) -> BSTBaseFilterer:


C. The filterer and sorter arguments were moved into kwargs. They were intended to be the "client" sorter/filterer settings. The server version of these arguments is "private", so from the perspective of these BSTColumn classes, they are just referred to as "sorter" and "filterer". The "field" argument is positional in the BSTSorter and BSTFilterer classes, but those values are autimatically set in these Column classes, so I made them into kwargs.

hepcat72 · 2025-04-06T20:00:19Z

DataRepo/views/models/bst_list_view/column/field.py

-            Data Format Name
-            Data Type Name
+            Data Format
+            Data Type


C. The BSTColumn class basically only supports fields directly on the root model. Those fields can be related fields (i.e. foreign keys), but they are not supported for search and sort, because for that, they need a "display field", which is what BSTRelatedColumn provides. So in this class, without that support, just displays the related model objects as they render in string context, but since searching and sorting on those fields uses their numeric primary keys, it automatically disables search and sort, because what the user sees is not what search/sort is based on.

So in this doc string, data_type__name is not supported. I changed this example to refer to only the foreign key field (thus the removal of "Name" from the header, and moved that example into BSTRelatedColumn.

hepcat72 · 2025-04-06T20:01:09Z

DataRepo/views/models/bst_list_view/column/field.py

+            len(path_tail) == 2
+            and path_tail[1] == "name"
+            and is_unique_field(self.field)
+        ):
            return underscored_to_title(path_tail[0])

        # Default is to use the last 2 elements of the path
        return underscored_to_title("_".join(path_tail))



C. As mentioned in base.py, I simplified these methods.

hepcat72 added 3 commits April 5, 2025 07:43

Added BSTRelatedColumn and made field an instance member of BSTColumn.

26fa93c

Also tweaked some of the BSTColumn doc string. Files checked in: DataRepo/models/utilities.py DataRepo/views/models/bst_list_view/column/field.py DataRepo/views/models/bst_list_view/column/related_field.py

hepcat72 added type:feature New feature or request priority:2-high Must be implemented in order to meet release goals status:inreview Issue/PR awaiting review/approval. effort:4-under1day An issue that would take less than a day to complete component:views View classes/methods labels Apr 5, 2025

hepcat72 self-assigned this Apr 5, 2025

hepcat72 commented Apr 5, 2025

View reviewed changes

hepcat72 added 2 commits April 5, 2025 10:47

Fleshed the BSTRelatedColumn.get_default_display_field docstring to e…

5490634

…xplain how a default is chosen. Files checked in: DataRepo/views/models/bst_list_view/column/related_field.py

hepcat72 commented Apr 6, 2025

View reviewed changes

hepcat72 mentioned this pull request Apr 8, 2025

BSTManyRelatedColumn #1501

Open

7 tasks

hepcat72 changed the title ~~BSTRelatedColumn~~ BSTRelatedColumn Apr 8, 2025

hepcat72 mentioned this pull request Apr 8, 2025

Integrated base class support for foreign keys #1499

Open

7 tasks

hepcat72 removed the status:inreview Issue/PR awaiting review/approval. label May 29, 2025

		@@ -354,7 +354,6 @@ def field_path_to_model_path(


		def is_key_field(
		model: Optional[Type[Model]],

		def assertNotWarns(unexpected_warning=UserWarning):
		def assertNotWarns(unexpected_warning=Warning):

		sorter = kwargs.get("sorter")
		filterer = kwargs.get("filterer")

		@@ -10,6 +10,8 @@
		from django.utils.functional import classproperty
		from django.utils.safestring import mark_safe

		@@ -128,22 +131,18 @@ def __init__(

		super().__init__(name, **kwargs)

		)


		@override_settings(DEBUG=True)

BSTRelatedColumn #1500

Are you sure you want to change the base?

BSTRelatedColumn #1500

Uh oh!

Conversation

hepcat72 commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary Change Description

Affected Issues/Pull Requests

Reviewer Notes/Requests

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

`BSTRelatedColumn` #1500

`BSTRelatedColumn` #1500

hepcat72 commented Apr 5, 2025 •

edited

Loading