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

The linkify kwarg doesn't support adding a querystring the way LinkColumn's render_link method did. #687

Open
benmehlman opened this issue Sep 6, 2019 · 6 comments

Comments

@benmehlman
Copy link

benmehlman commented Sep 6, 2019

Prior to the addition of linkify it was possible to customize the behavior of LinkColumn by overriding the render_link method. I did this, implementing a LinkTemplateColumn which supported the passing of a querystring template to be rendered and appended to the url.

I am now upgrading tables2 and see that LinkColumn has been changed to a wrapper and the url rendering has been moved to LinkTransform where it can't be overridden.

Three possible ways to get around this:

  1. Add querystring rendering capabilities to the LinkTransform class through an extra entry in the linkify dict (ideally this would support templating).

  2. Make the LinkTransform class swappable by adding a layer of indirection, eg.

     class Column():
         link_transform = LinkTransform
     
     MyLinkTransform(LinkTransform):
         pass
    
     Column.link_transform = MyLinkTransform
    

    (or we could get fancier than that...)

  3. Make Column.link into a method that could be overridden, eg.

    def link_transform(self, **kwargs)
        return  LinkTransform(attrs=self.attrs.get("a", {}), **lkwargs)
    

The alternatives (for me) that do not involve changes to tables2:

  1. I could subclass the existing LinkTransform class, and implement querystring functionality.. then override its callable so that it would return a bare url with my querystring appended. Then pass an instance of this as the value of linkify. At least that way I take advantage of the existing linkify code and keep current with any changes to it.

  2. Or I could simply make my own callable that renders the url, bypassing tables2's url capabilities, and pass that as the value of linkify.. but that's no fun, is it???

Thoughts?

@benmehlman
Copy link
Author

benmehlman commented Sep 6, 2019

So, I worked on this a little bit, and here's my example of an approach that does not change tables2:

from django_tables2.columns.base import LinkTransform

class LinkTemplate(LinkTransform):
    def __init__(self, url_name, template='{{u}}', args=None):

        super(LinkTemplate, self).__init__(reverse_args=(url_name, args))
        self.template = template

    def __call__(self, record, value):
        uri = self.call_reverse(record)
        return mark_safe(Template(self.template).render(Context({ 'r': record, 'v': value, 'u': uri })))

    @property
    def func(self):
        def f(record, value):
            return self(record, value)
        return f

And to use it (actual example from my application):

num_payments = tables.Column(
    linkify=LinkTemplate('payment_list', '{{u}}?account={{r.order__account__id}}&payer_name={{r.payer_name|urlencode}}').func,
    verbose_name='# Payments'
)

It is a little awkward looking but it works... It would be a lot neater if this functionality was part of the tables2 LinkTransform class.. if this idea would be accepted I will write the patch!

Thanks
Ben

PS: The reason I have to use the func property is because, even though the LinkTemplate instance is callable, python inspect does not recognize it as such because it doesn't derive from types.Function and call_with_appropriate throws a TypeError. So I had to wrap the call in a standard python function.

@benmehlman
Copy link
Author

benmehlman commented Sep 7, 2019

So, I had to go ahead with some kind of solution, so in case anyone's interested here's what I did. First, I created a subclass of tables2's LinkTransform that knows how to deal with query strings and fragment identifiers:

class QueryLinkTransform(LinkTransform):

    def __init__(self, **kwargs):
        self.query = kwargs.pop('query', None)
        self.fragment = self.query and self.query.pop('HASH', None)
        super(QueryLinkTransform, self).__init__(**kwargs)

    def compose_url(self, **kwargs):
        url = super(QueryLinkTransform, self).compose_url(**kwargs)

        if self.query:
            record = kwargs['record']
            url += '?' + urllib.urlencode({
                a: v.resolve(record) if isinstance(v, Accessor) else v
                for a, v in self.query.items()
            })
        if self.fragment:
            url += '#' + self.fragment

        return url

Then I wrote a function which returns a callback function that generates the url using that class:

def querylink(url_name, url_args=None, **query):
    qlt = QueryLinkTransform(reverse_args=(url_name, url_args), query=query)

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

    return callback

This supports Accessors for the querystring values, but sometimes I needed to do some work on the query string values using template filters (I was doing that before, mostly the date filter).. so I made a custom accessor that lets me use template code as an accessor if I want to use a filter or do something fancy:

class BangoAccessor(Accessor):
    def resolve(self, context, safe=True, quiet=False):
        if '{{' in self:

            c = Context(context if isinstance(context, dict) else None)
            c.update({ 'record': context, 'r': context })

            return Template(self).render(c)
        return super(BangoAccessor, self).resolve(context, safe, quiet)

A = BangoAccessor

The way this is used is as follows:

    mycolumn = Column(linkify=querylink(
           'url_name', 
           [1, A('pk')], 
           x=A('x'), y=A('y'), date=A('{{d|date:"Y-m-d"}}'),
           HASH='mysection'),
       verbose_name='My Column'
    ) 

And the generated URL would look something like:

/the/path/1/999/?x=xvalue&y=yvalue&d=2019-09-06#mysection

I didn't support kwargs for reverse because I didn't need them (yet).. and my choice to do it this way has a lot to do with me being in a hurry to get work done, and I didn't want to generate tons of patches for tables2 without any discussion first.. so this solves my problems without changing tables2 (except by subclassing). But as you can see it would not be hard to incorporate these ideas right into tables2.

@jieter
Copy link
Owner

jieter commented Sep 23, 2019

@benmehlman Thanks for opening the issue and sharing your thoughts/experiments.

I'd like to be able support more aspects of an url (query/hashtag) in the LinkTransform, but I'm not sure about the templating. This adds another bunch of template instances (could may be optimised to have one Template instance per Accessor instance, but certainly overhead).

A patch adding something like that is very welcome!

@benmehlman
Copy link
Author

Thanks for reading through it, I'm happy to write a patch for LinkTransform. I have one question there, which is to use six.moves for urllib/urllib2 import? Or forget py2 altogether?

For the template accessor, yes you're totally right, there is no point instantiating Template each time the accessor is rendered, it could be instantiated in Accessor's constructor since it never changes. Only the Context changes each time. If I do it that way would you accept that patch?

@jieter
Copy link
Owner

jieter commented Sep 23, 2019

We do not support python 2 anymore, so you can just import from urllib.

Let's do the version without templating first, and if we are happy with that take a look at the templating.

@benmehlman
Copy link
Author

This adds 'query' and 'fragment' options to linkify= {}

#711

Hope you will also take a look at my already subsmitted pr for issue #682 which I am currently using in our production. I would like to get off my fork and back to master if possible. Thanks!

benmehlman added a commit to benmehlman/django-tables2 that referenced this issue Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants