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

dynamic widget parameters for CharField #1637

Conversation

matthewhegarty
Copy link
Contributor

@matthewhegarty matthewhegarty commented Sep 22, 2023

Closes #1485

Dynamic widget parameters for CharField allows for storing 'None' in xlsx as empty string.

When Resource is created, if the underlying field is CharField or Subclass, and if it allows empty strings, then create kwargs which get passed to the widget, which allow for storing of empty strings.

See also #1623

Acceptance Criteria

  • Added integration tests
  • Manual testing of the steps defined in the issue

@matthewhegarty matthewhegarty changed the title dynamic widget parameters for CharField fixes 'NOT NULL constraint' e… dynamic widget parameters for CharField Sep 22, 2023
@matthewhegarty matthewhegarty changed the title dynamic widget parameters for CharField dynamic widget parameters for CharField Sep 22, 2023
@matthewhegarty matthewhegarty linked an issue Sep 22, 2023 that may be closed by this pull request
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.

I think I'm in alignment here.

Long term I think we may want to have some dialog about formats handling of nulls and try to structure that a bit better, but I think you're going in the right direction.

issubclass(django_field.__class__, fields.CharField)
and django_field.blank is True
):
widget_kwargs.update({"coerce_to_string": True, "allow_blank": True})

Choose a reason for hiding this comment

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

How will this behave if null and blank are both true? My naive reading is both nulls and blanks will end up as empty strings and come back in as empty strings, and this is universal across all formats (whether they support nulls or not).

I think this is a reasonable assumption, and blank+null handling is problematic as heck in Django already.

From what I am reading Django really discourages using both blank and null, and prefers blanks for text type fields. So we're following best practice if I"m reading right.

-- Basically, if the field allows blanks, we treat all nulls as blanks on export, and empty fields/cells as blanks on import. --

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Ryan, thanks for reviewing, much appreciated.

How will this behave if null and blank are both true?

'blank' is the overriding setting - so it will store None as empty string.

Basically, if the field allows blanks, we treat all nulls as blanks on export, and empty fields/cells as blanks on import.

Yes, this is the nub of it. This change only affects imports. There are other changes planned to make exports export either string or raw type.

Users can override the behaviour added in this PR by declaring their own Field, e.g. this declaration in core/admin.py will cause the original error to re-occur.

    author_email = fields.Field(
        attribute="author_email",
        widget=widgets.CharWidget(allow_blank=False, coerce_to_string=False))

@matthewhegarty matthewhegarty merged commit a9de5b1 into django-import-export:release-4 Sep 24, 2023
11 checks passed
@matthewhegarty matthewhegarty deleted the issue-1485-null-xlsx-field branch September 24, 2023 15:29
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 this pull request may close these issues.

NOT NULL constraint failed for non-nullable Charfields in xlsx
2 participants