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

Exporting fields with changed column_name doesn't work in v4 #1846

Closed
PetrDlouhy opened this issue May 20, 2024 · 15 comments · Fixed by #1861
Closed

Exporting fields with changed column_name doesn't work in v4 #1846

PetrDlouhy opened this issue May 20, 2024 · 15 comments · Fixed by #1861
Labels

Comments

@PetrDlouhy
Copy link
Contributor

Describe the bug
Fields like email = Field(column_name="recipientEmail") from my ModelResource are missing in the CSV.

To Reproduce
Steps to reproduce the behavior:

  1. Add ModelResource like:
class WisePayoutResource(ModelResource):
    note_recipient = Field(column_name="name")
    email = Field(column_name="recipientEmail")
    id = Field(column_name="paymentRefernece")
    reciever_type = Field(column_name="recieverType")
    amount_currency = Field(column_name="amountCurrency")
    amount = Field(column_name="amount")
    source_currency = Field(column_name="sourceCurrency")
    target_currency = Field(column_name="targetCurrency")
    type = Field(column_name="type")

    class Meta:
        model = models.Payout
        export_order = (
            "note_recipient",
            "email",
            "id",
            "reciever_type",
            "amount_currency",
            "amount",
            "source_currency",
            "target_currency",
            "type",
        )

    def dehydrate_source_currency(self, payout):
        return "USD"

    def dehydrate_target_currency(self, payout):
        return "USD"

    def dehydrate_amount_currency(self, payout):
        return "source"

    def dehydrate_note_recipient(self, payout):
        return NOTE_RECIPIENT

    def dehydrate_reciever_type(self, payout):
        return "PERSON"

    def dehydrate_email(self, payout):
        return payout.get_payout_data()["payout_email"]

    def dehydrate_amount(self, payout):
        return payout.amount

    def dehydrate_id(self, payout):
        return payout.id

    def dehydrate_type(self, payout):
        return "EMAIL"
  1. Export the table (e.g. Payout) with this resource and all columns enabled.
  2. In the resulting CSV are only columns amount and type (those that didn't change name of the column).

Versions (please complete the following information):

  • Django Import Export: 4.0.3
  • Python 3.11
  • Django 4.2

Expected behavior
All columns should be exported.

Additional context
Seems that the column name is not correctly matched in this case. @matthewhegarty I expect this would be easy for you to find the problem and fix it, but if not I can do some more digging and fixing.

@PetrDlouhy PetrDlouhy added the bug label May 20, 2024
@matthewhegarty
Copy link
Contributor

@PetrDlouhy - I can't reproduce it exactly in the way you have it.

  • What happens if you declare a fields list with the fields you want in the export?
  • I have a test case here
    • I find that if I remove the fields declaration, I get all Resource fields in the export.
    • Are you able to reproduce using this test case?

@PetrDlouhy
Copy link
Contributor Author

PetrDlouhy commented May 27, 2024

@matthewhegarty I think, that test is too specific for to test the actual problem. The test runs only ModelResource.export(), but the problem would probably lie in the admin form handling these field checkboxes:
Snímek obrazovky_2024-05-27_16-02-11

I expect that it behaves like if the checkbox for a field was not selected.

I will try to investigate further and make the appropriate test.

@matthewhegarty
Copy link
Contributor

matthewhegarty commented May 27, 2024

Hi Petr
I tried with your branch but reverted all your changes from resources.py. I added a new EBook and set the author to "Ian Fleming".

When I export the Ebook, I see:

image

The export file I get is:

id,Email of the author,name,published_date
1,test@example.com,Moonraker 2,2024-05-27

So it's possibly a bug that the Export HTML doesn't show the name of the field name to be exported ("Email of the author"), but other than that it looks ok... Am I missing something?

@PetrDlouhy
Copy link
Contributor Author

@matthewhegarty I didn't touch that part and I am not sure, which variant is correct. The original field name can be more readable for the user as it is/can be consistent across multiple resources.
I can imagine, that some resources would have totally unreadable column_names, e.g. IDs or legal identifiers.

Maybe we should display both values like Author Email (Email of the author).

@PetrDlouhy
Copy link
Contributor Author

@matthewhegarty I have added that functionality as separate PR #1857. Please check it and decide if you like this style or if I should prefer only one variant.

@matthewhegarty
Copy link
Contributor

matthewhegarty commented May 29, 2024

I notice that if I add the attribute name, then the test in #1857 passes:

# test fails
author = fields.Field(column_name="AuthorFooName")

# test passes
author = fields.Field(attribute="author", column_name="AuthorFooName")

This is similar to what was reported here this could be a v4 regression, I'll test in v3.

@matthewhegarty
Copy link
Contributor

I tested in v3 (added test here). This passes in v3, so we have a regression in v4 when the attribute name is not supplied.

@PetrDlouhy
Copy link
Contributor Author

@matthewhegarty I don't understand entirely. The test in will pass after applying #1856 (not #1857).

Is there something that needs to be fixed in either of those commits?
But maybe I am missing something, I will look at it tomorrow with fresh mind.

@matthewhegarty
Copy link
Contributor

Hi, I am still looking into it. I think there are a couple of things going on with other parts of the code which need addressing. I'll write a more detailed response tomorrow.

@matthewhegarty
Copy link
Contributor

hi Petr
I adapted your changes and made a new PR (#1861). I found that if I set an attribute on the field during initialization, then this resolves the issue without needed to modify resources.py. I have kept your other changes in.

The reason this works is because the author field in test_export_fields_column_name does not contain a defined attribute, therefore it is ignored during export.

This also fixes an issue I uncovered whilst testing (#1860). I propose that I merge this PR instead of yours. What do you think?

@PetrDlouhy
Copy link
Contributor Author

PetrDlouhy commented May 30, 2024

@matthewhegarty That seems to be a better solution. I tested it and it seems to be working.

The PR is missing the refactoring made in this commit: 9d2cab3
Those changes are unrelated to the PR itself, so it is clear why they are not in the PR (although the commit is and then the changes are reverted, so I suggest it might be better to rebase it out).
I think that the refactoring would be still beneficial for django-import-export and I can make it in separate PR (after merging #1861), if you think it too.

@matthewhegarty
Copy link
Contributor

matthewhegarty commented May 30, 2024

I only removed the change so that I could be sure that changes to resources.py did not affect the fix in #1861.

There is some duplication in those methods, so feel free to submit a new PR to make the code clearer. I would recommend to hide any shared logic in a protected method.

I also wasn't comfortable with the new Field attribute called original_name, but we shouldn't need that now due to the change in #1861.

Thanks again for all your help in identifying and fixing this issue 👍

@PetrDlouhy
Copy link
Contributor Author

Yeah, the original_name was only a tool to transfer the field variable name. Since now it is in attribute we don't need it anymore. I will make the refactor PR after #1861 is merged.

@PetrDlouhy
Copy link
Contributor Author

@matthewhegarty Everything seems to be working. Thank you very much for your attention and all the work you put into this project.

@matthewhegarty
Copy link
Contributor

np - thanks for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants