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

[feature request] add ability to export __str__ version of foreign keys #7

Open
olivierdalang opened this issue Jul 16, 2019 · 5 comments

Comments

@olivierdalang
Copy link

Hi !

Awesome module that works great !

In many cases, it would be nice to have foreign keys represented as str rather than the key itself. It allows for easier analysis of data without requiring one further join step.

It would be nice if there was a way to choose how to export foreign key, maybe even be able to have both representations (in two columns).

Might work on a PR for this if you think it's a good addition.

Cheers,

Olivier

@olivierdalang
Copy link
Author

As a reference, here's a quick&dirty implementation of an admin action for this. Worth noting one should make sure to use prefetch_related to avoid very slow performance.

from tabular_export.core import export_to_excel_response, get_field_names_from_queryset

def export_to_excel_action_rel(self, request, queryset):
    field_names = get_field_names_from_queryset(queryset)
    return export_to_excel_response(
        self.model._meta.verbose_name_plural + ".xlsx",
        field_names,
        [[getattr(o, fld) for fld in field_names] for o in queryset],
    )
export_to_excel_action_rel.short_description = "Export to Excel (with relations)"

@acdha
Copy link
Member

acdha commented Jul 17, 2019

Hah, yes — I was just working on a comment suggesting that care should be taken if your __str__ does anything which triggers lookups.

What do you think the syntax should look like — I was wondering whether that's a new function or perhaps something like this:

Current demo:

def export(request):
    qs = MyModel.objects.filter(foo="…").select_related("…")
    headers, data = flatten_queryset(qs, field_names=['title'])
    return export_to_csv_response('custom_export.csv', headers, data)

Ideas:

Use a sentinel class which makes it very easy to tell the desired operation:

def export(request):
    qs = MyModel.objects.filter(foo="…").select_related("…")
    headers, data = flatten_queryset(qs, field_names=['title', Stringify('author')])
    return export_to_csv_response('custom_export.csv', headers, data)

… or maybe [overly so?] like the ORM, using the ORM __:

def export(request):
    qs = MyModel.objects.filter(foo="…").select_related("…")
    headers, data = flatten_queryset(qs, field_names=['title', 'author____str__'])
    return export_to_csv_response('custom_export.csv', headers, data)

… or simply do a two-tuple form where the second is a callable which does whatever you want with each object instance:

def export(request):
    qs = MyModel.objects.filter(foo="…").select_related("…")
    headers, data = flatten_queryset(qs, field_names=['title', ('author', lambda x: str(x))])
    return export_to_csv_response('custom_export.csv', headers, data)

@olivierdalang
Copy link
Author

Not sure about the suggestions :

  • Stringify() and lambda x: str(x) are a bit ambiguous, as the syntax lets think that it would convert the field (a primary key) to string. It's not clear we're talking about the related instance.
  • the relation____str__ lookup is a bit an abuse of the lookup syntax, as lookups aren't at the same level than model logic (plus that's a lot of underscores).
  • all of them require to explicitly list the fields, not sure how it would work when you want to export all fields.

If we want to keep just one function, why not add a parameter, such as readable_foreign_keys or relations_as_text ?

In any case, I think it would require two admin actions, as you can't pass arguments to the admin actions.

Contrary to my implementation, I'd suggest to keep the original foreign key in the export, as __str__ may not be unique and thus prevent from properly using the exported data (e.g. in pivot tables).

@acdha
Copy link
Member

acdha commented Jul 17, 2019

Stringify() and lambda x: str(x) are a bit ambiguous, as the syntax lets think that it would convert the field (a primary key) to string. It's not clear we're talking about the related instance.

That's definitely just a first pass on naming — maybe it should have been something like StringifyRelatedField() in the first case? — but the one I'm thinking after more time is a slight variation on the second: ('author', lambda obj: str(obj.author)) as that would both make it clearer which object you're receiving and give the possibility of using other fields if that makes sense, and it gives you the most flexibility whereas a separate class would need variations for whatever other uses people come up with.

The main reason I was leaning against having a parameter or a separate function was unintended consequences: if you have more than one related field, you might not want to have all of them do this (e.g. I have a bunch of data which foreign-keys Language, which uses the 3 character ISO 639 code which I'd want to export as is) and you might not immediately notice that, say, while performance was fine at first it fell off when someone added another field to the list which wasn't covered by your existing prefetching.

Contrary to my implementation, I'd suggest to keep the original foreign key in the export, as str may not be unique and thus prevent from properly using the exported data (e.g. in pivot tables).

This is one of the other things I was wondering about with the syntax above. If I'm thinking about this correctly, it seems like we could have something like this:

def export(request):
    qs = MyModel.objects.filter(foo="…").select_related("…")
    headers, data = flatten_queryset(
        qs,
        field_names=[
            "title",
            "language",  # Will export primary key — e.g. “eng” or “fra”
            ("Language Name", lambda my_model: str(my_model.language)), # Will export as __str__ output for the related model
        ],  
    )
    return export_to_csv_response("custom_export.csv", headers, data)

@olivierdalang
Copy link
Author

What about having author saving the instance's __str__ and author_id saving the actual key? Those are already accessible from the instance and are quite straightforward.

By default, if we don't want a parameter, we could just output both.

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