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

Send null for string like columns that don't allow empty strings when cell is cleared #4030

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

aniketkulkarni17
Copy link
Contributor

Fixes #3784

Technical details

  • When a cell is cleared, the frontend will send "null" instead of "" to the backend, thus allowing empty values for the following column datatypes:
    • Date
    • Date and time
    • Time
    • Email
    • URI

Screenshots

  • Before
    Screenshot 2024-11-19 214938
    Screenshot 2024-11-19 214954

  • After
    Screenshot 2024-11-19 214852
    Screenshot 2024-11-19 214907

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@aniketkulkarni17
Copy link
Contributor Author

@seancolsen does this fit the requirement? Please let me know if any changes are required.

@seancolsen seancolsen self-assigned this Nov 19, 2024
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Nov 19, 2024
@seancolsen seancolsen added this to the Backlog milestone Nov 19, 2024
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @aniketkulkarni17. This kind of works, but has some issues...

Issues

  • For Email and URI columns, the front end is sending null to the API (good). But it doesn't display NULL to the user afterwards. If you click Refresh, then you see NULL, but that's not great UX.

  • Database types like time without time zone are no included in your string set and thus don't receive this fix. We could potentially address that by adding more database types to your string set. But I'd rather fix this at a lower level.

  • This PR has a failing test because the code in your changes is not formatted correctly. We use Prettier to automatically format code. You can read more in our docs about using it. Personally, I have VS Code configured to format on save which I love.

    docker exec -it -w /code/mathesar_ui mathesar_service_dev npx prettier --write .
    

    I'd like you to take a different direction with the overal implementation of this PR, but when doing so, make sure to run Prettier at some point in your workflow and commit the changes it makes. The changes from Prettier don't need to be in a separate commit but it's also fine if they are.

New direction

I said I'd like you to fix this at a lower level, and part of the reason is that the UI for inputting a cell value actually gets used in a lot of places. Not just within the sheet, but also the record page, the default value setting, the filter conditions on the table page, the filter transformations within the data explorer, the record selector etc. Some of those might be unaffected by this bug due to higher-level logic that handles null values, but suffice to say: the clean way to handle this is to do it at a low level, per-type.

(As a disclaimer, I'll say that this work touches on some of the messiest and hard-to-work-with parts of our front end codebase, so, sorry about that! You're going to end up on somewhat of a wild goose chase.)

Recall that I said:

Interestingly, the Duration type actually already works as expected.

Let's dig deeper to see why it works for Duration...

  1. Go to src/stores/abstract-types/type-configs/ and you'll see one file for each UI type. In the code, a "UI Type" is called an "AbstractType". But the idea is that each UI type potentially maps to multiple database types. And one database type maps to one UI type. The default object exported from each of those files defines the UI type. Look at the cellInfo.type property, and you'll see the name of the cell type that we use to work with this UI type. So in duration.ts we see a cellInfo.type of 'duration'. Good. (But other UI types, like Email won't necessarily line up one-for-one like this!)

  2. Now if you look in the src/components/cell-fabric/data-types/ directory, you'll see files which match up with those cell types. So within duration.ts (same file name, different directory) you'll see that getInput() is returning a FormattedInputProps component with props that specify that the formatter is to be a DurationFormatter. Ok cool. So, what is a DurationFormatter, and how does it work?

  3. In src/utils/duration/DurationFormatter.ts, we're specifying a parse() function. That's the entry point to the logic that translates the string the user entered into a standardized value. Notice that within ParseResult the value can be T | null, meaning that the return value of the parse function should be null to represent a NULL cell value. Cool. So where is the logic that returns null for an empty string input?

  4. Higher up in DurationFormatter.ts, we have this

    let cleanedInput = userInput.trim();
    if (cleanedInput === '') {
      return null;
    }

    Boom! That' exactly what we want to see. Not only does it translate an empty string to a null value, it also trims the string first. Brilliant!

So, what I'd like you to do to fix this issue is walk through the same steps above but for the problematic UI Types (not database types). So if we start with the Date type, we'd go to step 1. and navigate to src/stores/abstract-types/type-configs/. See that cellInfo.type is date. Then go to src/components/cell-fabric/data-types/ and open date.ts, find the component returned from getInput(), look at its props, and eventually get to DateTimeFormatter which has a parse function that doesn't handle empty strings correctly. Wonderful. Here is where we need to make the fix for the Date type.

We could consider adding a new abstraction like some sort of intermediate formatter that automatically handles empty strings, and then we'd use that formatter as a basis for the custom formatters used in these types. But I'm not sure that's worth it at this stage.

URI and Email are going to be a little trickier, so leave those for last. We'll probably need to add a new email.ts file in src/components/cell-fabric/data-types/. I'm not entirely sure.

@seancolsen seancolsen added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Nov 20, 2024
@seancolsen
Copy link
Contributor

@aniketkulkarni17 are you still planning to work on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: revision A PR awaiting follow-up work from its author after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send null for string-like columns that don't allow empty strings when cell is cleared
2 participants