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

Ar/support sorting by datetime #1

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

amandawraymond
Copy link
Collaborator

@amandawraymond amandawraymond commented Jan 17, 2023

Problem

Reported DateTime Issue

  • failure to correctly paginate through records when sorting by a DateTime selector

Solution

ensure the serialization and deserialization via JSON of the Cursors handle the precision of DateTime data down to the nanosecond

Details

Copy link

@colintsteele colintsteele left a comment

Choose a reason for hiding this comment

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

Should work for our purposes, but I do wonder about only checking that the custom order field is a hash before digging into keys that could potentially (albeit not currently) not exist.

Comment on lines 204 to 206
it 'decodes the string succesfully' do
expect(decoded.id).to eq record.id
expect(decoded.order_field_value).to eq record.created_at
end

Choose a reason for hiding this comment

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

I can't personally think of a way to do this, but it would be nice to have an assertion that tests nanosecond precision explicitly, instead of relying on an equality check on two Time objects, which inherently have that precision built in. Idk, I always hated testing timestamps so I think this is good.

"The given cursor `#{encoded_string}` was decoded as " \
"`#{decoded}` but could not be parsed"
end
if decoded[0].is_a?(Hash)

Choose a reason for hiding this comment

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

decoded[0] could be turned into a hash for reasons other than this one right? So is it worth adding a check here for seconds and nanoseconds keys?

if decoded[0].is_a?(Hash) && ['seconds', 'nanoseconds'].all? { |key| decoded[0].key? key }

@amandawraymond amandawraymond force-pushed the ar/support_sorting_by_datetime branch 5 times, most recently from 76c0d62 to 59949bb Compare January 27, 2023 18:04
…rsors add spec coverage for timestamp order_by selector in pagination fetch

Bump version to 0.3.1
@@ -49,7 +49,7 @@ jobs:
runs-on: ubuntu-20.04
strategy:
matrix:
ruby-version: ['2.6', '2.7', '3.0', '3.1']
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

versions fail on workflow

@@ -1,5 +1,5 @@
# frozen_string_literal: true

module RailsCursorPagination
VERSION = '0.3.0'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bump version

@@ -23,18 +24,19 @@ class Post < ActiveRecord::Base; end
ActiveRecord::Migration.verbose = ENV.fetch('VERBOSE', nil)

ActiveRecord::Base.establish_connection(
adapter: ENV.fetch('DB_ADAPTER', 'mysql2'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

default to postgres

# Ensure we have an empty `posts` table with the right format
ActiveRecord::Migration.drop_table :posts, if_exists: true

ActiveRecord::Migration.create_table :posts do |t|
t.string :author
t.string :content
t.timestamps
Copy link
Collaborator Author

@amandawraymond amandawraymond Jan 27, 2023

Choose a reason for hiding this comment

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

add timestamped columns to compare in specs

@amandawraymond amandawraymond merged commit d4a1333 into master Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants