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

Default to read only field on import if only one file format is defined #1690

Conversation

matthewhegarty
Copy link
Contributor

@matthewhegarty matthewhegarty commented Nov 22, 2023

Problem

Closes #1682

If only one file format is defined in IMPORT_FORMATS, then display a read only field:

settings.py:

from import_export.formats.base_formats import CSV, XLSX
IMPORT_FORMATS = [CSV]

image

This makes the import process consistent with the export process.

Solution

Update the form to add extra logic to handle read-only fields.
Update the import.html template.

Acceptance Criteria

  • Integration tests
  • Manual testing

@matthewhegarty matthewhegarty changed the base branch from main to release-4 November 22, 2023 17:42
@matthewhegarty matthewhegarty marked this pull request as ready for review November 24, 2023 16:37
Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

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

Nice quality of life improvement. my code comment is not a blocker.

@@ -41,6 +41,13 @@ def __init__(self, import_formats, *args, **kwargs):
choices.insert(0, ("", "---"))

Choose a reason for hiding this comment

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

This is a super minor nit, but I think this if chain could be a little cleaner using a guard clause pattern:

if not import_formats: 
    raise ValueError("Invalid import formats list"  
if len(input_formats) == 1:
   <do label stuff>  
if len(input_formats) >= 1:
   <do guess_format stuff> 

It looks to me like the ".choices" might be redundant (or only used in input_formats >1 clause) so could be moved inside that ifcheck as well--but I'm not sure there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I looked into this and decided that the forms module could do with a refactor to avoid duplication. This means that I have added a few more changes so that the forms classes are standardised.

I'd be grateful if you could take a look.

Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

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

This looks like a solid quality of life! My code comment is non-blocking

Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

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

This refactor looks good to me. Lots of moving bits so possible to miss something, but hey, tests :) Good work!

@matthewhegarty matthewhegarty merged commit a410a60 into django-import-export:release-4 Nov 26, 2023
11 checks passed
@matthewhegarty matthewhegarty deleted the issue-1682-default-import-order branch November 26, 2023 15:54
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

Successfully merging this pull request may close these issues.

Default to read-only field on import if a single Resource or file format is defined
2 participants