Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #687 Added ability to specify query and fragment elements of url via linkify. #711

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 62 additions & 19 deletions django_tables2/columns/base.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from collections import OrderedDict
from itertools import islice
from urllib.parse import urlencode

from django.core.exceptions import ImproperlyConfigured
from django.urls import reverse
Expand Down Expand Up @@ -65,7 +66,9 @@ class LinkTransform:
accessor = None
attrs = None

def __init__(self, url=None, accessor=None, attrs=None, reverse_args=None):
def __init__(
self, url=None, accessor=None, attrs=None, reverse_args=None, query=None, fragment=None
):
"""
arguments:
url (callable): If supplied, the result of this callable will be used as ``href`` attribute.
Expand All @@ -75,10 +78,14 @@ def __init__(self, url=None, accessor=None, attrs=None, reverse_args=None):
reverse_args (dict, tuple): Arguments to ``django.urls.reverse()``. If dict, the arguments
are assumed to be keyword arguments to ``reverse()``, if tuple, a ``(viewname, args)``
or ``(viewname, kwargs)``
query (dict): If supplied, field-value pairs to be formatted into a query string.
fragment (str): If supplied, value of URL fragment identifier (hash).
"""
self.url = url
self.attrs = attrs
self.accessor = accessor
self.query = query
self.fragment = fragment

if isinstance(reverse_args, (list, tuple)):
viewname, args = reverse_args
Expand All @@ -95,23 +102,37 @@ def compose_url(self, **kwargs):
record = kwargs["record"]

if self.reverse_args.get("viewname", None) is not None:
return self.call_reverse(record=record)

if bound_column is None and self.accessor is None:
accessor = Accessor("")
url = self.call_reverse(record=record)
else:
accessor = Accessor(self.accessor if self.accessor is not None else bound_column.name)
context = accessor.resolve(record)
if not hasattr(context, "get_absolute_url"):
if hasattr(record, "get_absolute_url"):
context = record
if bound_column is None and self.accessor is None:
accessor = Accessor("")
else:
raise TypeError(
"for linkify=True, '{}' must have a method get_absolute_url".format(
str(context)
)
accessor = Accessor(
self.accessor if self.accessor is not None else bound_column.name
)
return context.get_absolute_url()
context = accessor.resolve(record)
if not hasattr(context, "get_absolute_url"):
if hasattr(record, "get_absolute_url"):
context = record
else:
raise TypeError(
"for linkify=True, '{}' must have a method get_absolute_url".format(
str(context)
)
)
url = context.get_absolute_url()

if self.query:
url += "?" + urlencode(
{
a: v.resolve(record) if isinstance(v, Accessor) else v
for a, v in self.query.items()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this does not work for QueryDicts, which is needed to support multiple values for a single key (i.e. id=1&id=2). I'm not sure if thats worth it to add support for.

Copy link
Author

@benmehlman benmehlman May 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I am back! Are you still interested in this patch?
If the only think holding it back is support for QueryDict I can make that change.

Or.. I should ask.. is it actual QueryDict support you would like (direct usage of QueryDict class? Or just equivalent functionality, eg. a list would be interpreted as multiple values:

myquery = {
     "a": 1,
     "b": [2, 3, 4, 5]
}

Result: ?a1&b=2&b=3&b=4&b=5

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome back ;)

Yes, when looking at it now, multiple values is a must-have.

}
)
if self.fragment:
url += "#" + self.fragment

return url

def call_reverse(self, record):
"""
Expand Down Expand Up @@ -150,6 +171,20 @@ def __call__(self, content, **kwargs):

return format_html("<a {}>{}</a>", attrs.as_html(), content)

@classmethod
def get_callback(cls, *args, **kwargs):
"""
This method constructs a LinkTransform and returns a callback function suitable as the linkify
parameter of ``Column.__init__()`` This may be used if you wish to subclass LinkTransform or to access
(future) features of LinkTransform which may not be exposed via ``linkify``.
"""
link_transform = cls(*args, **kwargs)

def callback(record, value, **kwargs):
return link_transform.compose_url(record=record, value=value, **kwargs)

return callback


@library.register
class Column:
Expand Down Expand Up @@ -207,9 +242,11 @@ class Column:
``a`` tag. The different ways to define the ``href`` attribute:

- If `True`, the ``record.get_absolute_url()`` or the related model's
`get_absolute_url()` is used.
- If a callable is passed, the returned value is used, if it's not ``None``.
- If a `dict` is passed, it's passed on to ``~django.urls.reverse``.
``get_absolute_url()`` is used.
- If a callable is passed, the returned value is used, if it's not `None`.
- If a `dict` is passed, optional items named `query` (dict), and `fragment` (str),
if present, specify the query string and fragment (hash) elements of the URL.
The remaining items are passed on to ``~django.urls.reverse`` as kwargs.
- If a `tuple` is passed, it must be either a (viewname, args) or (viewname, kwargs)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List is also supported per your change in line 326, so can you add it to the docs here too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will do, but I guess this is all going to take some time.. because we are on Debian Jessie here w/ Python 3.4. So I can run flake8, but can't run black (required python3.6) and tables2 tests fail due to too-old version of django_filters. We are in the process of a upgrading but it will take some time. Probably I'll just have to create a debian stretch VM for tables2 development.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just push however many times you like to let the CI do it, the logs can be inspected here:

https://travis-ci.org/jieter/django-tables2/pull_requests?utm_medium=notification&utm_source=github_status

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but if I have to write tests I need to be able to run them locally.. so I set up the environment. I've got several other tables2 features I want to offer to the project that I am using here, some implemented using monkeypatches that I have had in place for years, and I just recently found a problem with pagination and order_by that I want to fix (in the meantime, I coded around it by eliminating my use of RequestConfig and overriding SingleTableMixin.get_table()), so I might as well set things up correctly.

So now I've been able to run black and see what it did not like about my formatting, so that's fine now.

But I've got two questions:

  1. I've noticed that in the docstrings, you sometimes use single backticks around symbols or code (black monospace) and sometimes double backticks (red monospace). And I looked at the existing docs to see whether there was a rule to this but could not find a consistent rule. Trying to get this right so would appreciate your guidance here.

  2. When running tox, in your requirements file I see you specified Django==2.2.3, but it installs Django 3.1.. and this is a problem because:

jazzband/django-oauth-toolkit#752

So the tables2 tests all fail.

So there's legitimate breakage between tables2 HEAD with Django 3.1 and I can fix that, but that should probably be a separate pr... meanwhile I am stuck on the tests, so any help understanding why tox doesn't use the right django is appreciated.

This is my first time using tox so maybe I'm just running it wrong... I more or less followed what you did here: https://gist.github.com/jieter/e0c2f577fe3c685edd04

Thanks!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 1: I had to look this up, so it is probably not consistent:

The default role (content) has no special meaning by default. You are free to use it for anything you like, e.g. variable names; use the default_role config value to set it to a known role – the any role to find anything or the py:obj role to find Python objects are very useful for this.

http://www.sphinx-doc.org/en/master/usage/restructuredtext/roles.html

If we are going to adopt a rule, it would be some work to bring the existing docstrings into alignment with it, so I'd suggest to not worry about it too much for this PR.

For 2: Tox installs different versions of Django for different entries in the test matrix:

django-tables2/tox.ini

Lines 33 to 36 in 4941e4f

2.1: Django==2.1.*
2.2: Django==2.2.*
3.0: Django==3.0a1
master: https://github.com/django/django/archive/master.tar.gz

In addition to Django, it also installs a couple of other dependencies: from requirements/common.pip

tuple, which is also passed to ``~django.urls.reverse``.

Expand Down Expand Up @@ -295,8 +332,14 @@ def __init__(
link_kwargs = None
if callable(linkify) or hasattr(self, "get_url"):
link_kwargs = dict(url=linkify if callable(linkify) else self.get_url)
elif isinstance(linkify, (dict, tuple)):
elif isinstance(linkify, (list, tuple)):
link_kwargs = dict(reverse_args=linkify)
elif isinstance(linkify, dict):
# specific keys in linkify are understood to be link_kwargs, and the rest must be reverse_args
link_kwargs = {
name: linkify.pop(name) for name in ("query", "fragment") if name in linkify
}
link_kwargs["reverse_args"] = linkify
elif linkify is True:
link_kwargs = dict(accessor=self.accessor)

Expand Down
6 changes: 6 additions & 0 deletions django_tables2/columns/linkcolumn.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class LinkColumn(BaseLinkColumn):
text (str or callable): Either static text, or a callable. If set, this
will be used to render the text inside link instead of value (default).
The callable gets the record being rendered as argument.
query (dict): Optional name/value pair for query string. See `~urllib.urlencode`.
fragment (str): Optional URL Fragment Identifier (hash).

.. [2] In order to create a link to a URL that relies on information in the
current row, `.Accessor` objects can be used in the *args* or *kwargs*
Expand Down Expand Up @@ -130,6 +132,8 @@ def __init__(
kwargs=None,
current_app=None,
attrs=None,
query=None,
fragment=None,
**extra
):
super().__init__(
Expand All @@ -140,6 +144,8 @@ def __init__(
args=args,
kwargs=kwargs,
current_app=current_app,
query=query,
fragment=fragment,
),
**extra
)
Expand Down
25 changes: 25 additions & 0 deletions tests/columns/test_linkcolumn.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import re
from urllib.parse import parse_qsl, urlparse

from django.template import Context, Template
from django.test import TestCase
from django.urls import reverse
Expand Down Expand Up @@ -228,3 +231,25 @@ class Table(tables.Table):

table = Table([{"col": "link-text", "id": 1}])
self.assertEqual(table.rows[0].get_cell_value("col"), "link-text")

def test_query(self):
query = {"a": A("a"), "b": "baker", "c": "12"}

class PersonTable(tables.Table):
a = tables.LinkColumn("occupation", kwargs={"pk": A("a")}, query=query)

table = PersonTable([{"a": 0}, {"a": 1}])

for a in range(1):
href = re.search('href="(.*)"', table.rows[a].get_cell("a")).group(1)
query["a"] = str(a)
self.assertEqual(query, dict(parse_qsl(urlparse(href).query)))

def test_fragment(self):
class PersonTable(tables.Table):
a = tables.LinkColumn("occupation", kwargs={"pk": A("a")}, fragment="fragval")

table = PersonTable([{"a": 0}, {"a": 1}])

href = re.search('href="(.*)"', table.rows[0].get_cell("a")).group(1)
self.assertEqual("fragval", urlparse(href).fragment)