Split datetime into three UI types #1093

dmos62 · 2022-02-22T18:36:56Z

Fixes #960

Changes the definition of Mathesar/UI types. What was before datetime UI type, now is date, time and datetime types.

Technical details

Below description is outdated. See this comment: #1093 (comment)

@pavish @seancolsen these types' display options are different too. datetime initially accepted format display option. Now, datetime accepts date_format and time_format display options: date accepts date_format: and, time accepts time_format.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master 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
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.

…into-three-ui-types

…types

dmos62 · 2022-03-01T13:50:11Z

Dependent on #1089.

dmos62 · 2022-03-01T13:54:40Z

@silentninja I made some slight refactors to the display options serializer.

See https://github.com/centerofci/mathesar/pull/1093/files#diff-0cba368aff485c57e63a82055dfe283abac0a640d1896e087ae59d160210270cL105

and https://github.com/centerofci/mathesar/pull/1093/files#diff-0cba368aff485c57e63a82055dfe283abac0a640d1896e087ae59d160210270cR250

Could you give it a quick glance for a sanity check?

kgodey · 2022-03-01T14:06:42Z

@dmos62 can you mark as a draft since this isn't ready to be merged (due to other PRs needing to be merged first?)

dmos62 · 2022-03-01T15:30:29Z

@kgodey undrafting is part of the process to mark this as ready for review. Should I undraft and redraft in this case?

Edit: code review guidelines for authors don't account for the scenario where a PR shouldn't be merged, but can be reviewed.

dmos62 · 2022-03-01T15:39:44Z

Major changes after sync call. What frontend needs to know (first post is now outdated):

the datetime UI type was split into time, date, datetime;
time, date and datetime formats accept only the format display option;
format display option is validated on the backend to contain no more than 255 characters.

kgodey · 2022-03-01T15:47:58Z

undrafting is part of the process to mark this as ready for review. Should I undraft and redraft in this case?

Edit: code review guidelines for authors don't account for the scenario where a PR shouldn't be merged, but can be reviewed.

@dmos62 I would either:

leave it as draft and set status to review, assign reviewers manually, and post in the Code Review channel
not mark it as ready for review until it's actually ready to be merged and instead remind people to prioritize reviewing the PRs blocking the merge of the current PR.

Please update the code review guidelines in the wiki.

Also, I recommend basing as many PRs as you can off of master if what you're working on doesn't depend on functionality from previous PRs. You might already be doing that.

dmos62 · 2022-03-01T15:50:49Z

@kgodey I'll update the wiki.

Also, I recommend basing as many PRs as you can off of master if what you're working on doesn't depend on functionality from previous PRs. You might already be doing that.

I'm already doing that. Most PRs end up implementing something that will affect the other PRs. I also sometimes base off another dev branch to avoid having to resolve merge conflicts.

dmos62 · 2022-03-01T15:55:09Z

@silentninja I made some slight refactors to the display options serializer.

See https://github.com/centerofci/mathesar/pull/1093/files#diff-0cba368aff485c57e63a82055dfe283abac0a640d1896e087ae59d160210270cL105

and https://github.com/centerofci/mathesar/pull/1093/files#diff-0cba368aff485c57e63a82055dfe283abac0a640d1896e087ae59d160210270cR250

Could you give it a quick glance for a sanity check?

@silentninja after implementing changes requested in sync call, most of the refactor I mentioned has been removed, because most of the code it was refactoring isn't necessary anymore. You might want to take a look anyway, but now the code is pretty simple, so you're not missing out if you don't.

silentninja · 2022-03-01T16:59:22Z

mathesar/api/serializers/shared_serializers.py

-    serializers.Serializer
-):
-    format = serializers.CharField(validators=[TimeWithoutTimeZoneFormatValidator()])
+    format = serializers.CharField(validators=[is_time_format_valid])


It would be better to use the max_length parameter instead of custom validator, this would help up to throw errors with proper error codes.

Fixed 27f75ca

dmos62 · 2022-03-03T16:19:42Z

@kgodey should be ready for merge.

codecov-commenter · 2022-03-03T16:30:53Z

Codecov Report

Merging #1093 (890064a) into master (67e615a) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 890064a differs from pull request most recent head 837550f. Consider uploading reports for the commit 837550f to get more accurate results

@@            Coverage Diff             @@
##           master    #1093      +/-   ##
==========================================
- Coverage   93.38%   93.25%   -0.13%     
==========================================
  Files         112      112              
  Lines        4291     4243      -48     
==========================================
- Hits         4007     3957      -50     
- Misses        284      286       +2

Flag	Coverage Δ
pytest-backend	`93.25% <100.00%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
mathesar/api/display_options.py	`100.00% <ø> (ø)`
db/functions/hints.py	`94.73% <100.00%> (+0.29%)`	⬆️
db/types/base.py	`97.54% <100.00%> (+0.17%)`	⬆️
mathesar/api/serializers/shared_serializers.py	`78.08% <100.00%> (-7.97%)`	⬇️
mathesar/database/types.py	`97.67% <100.00%> (+0.05%)`	⬆️
mathesar/api/exceptions/mixins.py	`86.84% <0.00%> (-5.27%)`	⬇️
db/columns/operations/alter.py	`96.57% <0.00%> (-0.10%)`	⬇️
db/columns/operations/infer_types.py	`97.05% <0.00%> (-0.09%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67e615a...837550f. Read the comment docs.

kgodey · 2022-03-03T16:52:16Z

@dmos62 sounds good, assigned to @silentninja to merge since he already reviewed it.

silentninja

Also, remove the arrow library` as it is unnecessary. Please make the requested changes and merge.

silentninja · 2022-03-07T23:32:21Z

mathesar/api/serializers/shared_serializers.py



 class DurationDisplayOptionSerializer(MathesarErrorMessageMixin, OverrideRootPartialMixin, serializers.Serializer):
-    format = serializers.CharField(validators=[DurationFormatValidator()])
+    format = serializers.CharField(max_length=255, validators=[raise_if_duration_format_invalid])


Based on our conclusion, other than a character length check, we don't need any additional validators. So the validators field and the validation function should be removed

…rofci/mathesar into split-datetime-into-three-ui-types

dmos62 · 2022-03-08T13:06:20Z

Removed arrow library and custom time/duration format validation.

dmos62 added 4 commits February 22, 2022 17:25

Split UI datetime type

c9ff1d1

Add appropriate hints to time-related types

8b7fe63

Split datetime display options

c4dcaf4

Update hints

b0e5b07

dmos62 added affects: architecture Improvements or additions to architecture work: backend Related to Python, Django, and simple SQL status: draft labels Feb 22, 2022

dmos62 added this to the [07] Initial Data Types milestone Feb 22, 2022

dmos62 self-assigned this Feb 22, 2022

dmos62 added 3 commits February 28, 2022 16:35

Merge branch 'add-filters-for-money-and-boolean' into split-datetime-…

57852ad

…into-three-ui-types

Merge branch 'time-filter-aliases' into split-datetime-into-three-ui-…

f677487

…types

Finish adapting serializers

1519e6b

dmos62 changed the base branch from master to time-filter-aliases March 1, 2022 13:22

dmos62 added the pr-status: review A PR awaiting review label Mar 1, 2022

dmos62 marked this pull request as ready for review March 1, 2022 13:52

dmos62 requested a review from a team March 1, 2022 13:52

github-actions bot requested review from eito-fis, kgodey, mathemancer, pavish and silentninja and removed request for a team March 1, 2022 13:52

Revert diplay_options changes; reduce validation to checking length

3cc27eb

dmos62 marked this pull request as draft March 1, 2022 15:47

dmos62 removed the status: draft label Mar 1, 2022

silentninja reviewed Mar 1, 2022

View reviewed changes

kgodey mentioned this pull request Mar 1, 2022

Frontend implementation of Date & Time Mathesar types #260

Closed

1 task

dmos62 mentioned this pull request Mar 3, 2022

Introduce some filter aliases for time types #1101

Merged

7 tasks

Base automatically changed from time-filter-aliases to master March 3, 2022 15:38

dmos62 added 3 commits March 3, 2022 18:10

Merge branch 'master' into split-datetime-into-three-ui-types

a69ec81

Switch to defacto way to specify max_length

27f75ca

Merge branch 'master' into split-datetime-into-three-ui-types

d94580f

dmos62 marked this pull request as ready for review March 3, 2022 16:19

kgodey assigned silentninja Mar 3, 2022

Merge branch 'master' into split-datetime-into-three-ui-types

a3ed178

silentninja approved these changes Mar 7, 2022

View reviewed changes

silentninja removed their assignment Mar 7, 2022

silentninja 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 Mar 7, 2022

dmos62 added 3 commits March 8, 2022 15:04

Remove custom time/duration format validation

ea6050a

Remove arrow library

7918971

Merge branch 'split-datetime-into-three-ui-types' of github.com:cente…

890064a

…rofci/mathesar into split-datetime-into-three-ui-types

dmos62 requested a review from a team March 8, 2022 13:05

dmos62 requested a review from silentninja March 8, 2022 13:06

Merge branch 'master' into split-datetime-into-three-ui-types

837550f

silentninja enabled auto-merge March 8, 2022 13:10

silentninja merged commit 10fa641 into master Mar 8, 2022

silentninja deleted the split-datetime-into-three-ui-types branch March 8, 2022 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split datetime into three UI types #1093

Split datetime into three UI types #1093

dmos62 commented Feb 22, 2022 •

edited

Loading

dmos62 commented Mar 1, 2022 •

edited

Loading

dmos62 commented Mar 1, 2022

kgodey commented Mar 1, 2022

dmos62 commented Mar 1, 2022 •

edited

Loading

dmos62 commented Mar 1, 2022 •

edited

Loading

kgodey commented Mar 1, 2022

dmos62 commented Mar 1, 2022

dmos62 commented Mar 1, 2022 •

edited

Loading

silentninja Mar 1, 2022

dmos62 Mar 3, 2022

dmos62 commented Mar 3, 2022

codecov-commenter commented Mar 3, 2022 •

edited

Loading

kgodey commented Mar 3, 2022

silentninja left a comment

silentninja Mar 7, 2022

dmos62 commented Mar 8, 2022

Split datetime into three UI types #1093

Split datetime into three UI types #1093

Conversation

dmos62 commented Feb 22, 2022 • edited Loading

Checklist

Developer Certificate of Origin

dmos62 commented Mar 1, 2022 • edited Loading

dmos62 commented Mar 1, 2022

kgodey commented Mar 1, 2022

dmos62 commented Mar 1, 2022 • edited Loading

dmos62 commented Mar 1, 2022 • edited Loading

kgodey commented Mar 1, 2022

dmos62 commented Mar 1, 2022

dmos62 commented Mar 1, 2022 • edited Loading

silentninja Mar 1, 2022

Choose a reason for hiding this comment

dmos62 Mar 3, 2022

Choose a reason for hiding this comment

dmos62 commented Mar 3, 2022

codecov-commenter commented Mar 3, 2022 • edited Loading

Codecov Report

kgodey commented Mar 3, 2022

silentninja left a comment

Choose a reason for hiding this comment

silentninja Mar 7, 2022

Choose a reason for hiding this comment

dmos62 commented Mar 8, 2022

dmos62 commented Feb 22, 2022 •

edited

Loading

dmos62 commented Mar 1, 2022 •

edited

Loading

dmos62 commented Mar 1, 2022 •

edited

Loading

dmos62 commented Mar 1, 2022 •

edited

Loading

dmos62 commented Mar 1, 2022 •

edited

Loading

codecov-commenter commented Mar 3, 2022 •

edited

Loading