[NO-ISSUE] feat: allow to query multiple users (freebusy) #26

GuillaumeDecMeetsMore · 2024-07-29T09:18:04Z

Changed

[test] Add tests for querying freebusy on multiple calendars of the same user
[feat] Allow to query freebusy on multiple users (all calendars)
[test] Adapt test for handling Japanese event names
[fix] Adapt code for handling metadata the same way in all entities
[test] Adapt test for metadata

…d-integration-test-for-our-use-cases

…r-use-cases

…nal-user-uuid

…' into guillaume/feat/add-format-lint-via-biome

…e/refactor/convert-unix-timestamps-to-datetimes

…-to-datetimes

…low query metadata

…' into guillaume/feat/add-js-docs-client-lib

…eat/allow-to-query-free-busy-multiple

…ltiple

GuillaumeDecMeetsMore · 2024-08-15T06:15:14Z

justfile

@@ -1,4 +1,5 @@
 export DATABASE_URL := "postgresql://postgres:postgres@localhost:45432/nettuscheduler"
+export RUST_BACKTRACE := "1"


Print backtrace in case of error

GuillaumeDecMeetsMore · 2024-08-15T06:16:48Z

scripts/run_tests.sh

 # Run the migrations
 cd crates/infra && sqlx migrate run && cd ../..

 # Run the tests
-cargo nextest run --workspace && cd ..
+cargo test --workspace && cd ..


Unfortunately the tests should be run sequentially per crate as the DB instance is shared (nextest run all crates in parallel), so for the moment I revert back to cargo test (default test runner)

GuillaumeDecMeetsMore · 2024-08-15T06:18:43Z

scheduler/clients/javascript/lib/userClient.ts

+    return {
+      res: res.res,
+      status: res.status,
+      data: {
+        busy: res.data.busy.map(convertInstanceDates),
+      },
+    }


Fix, it should retun Dates and not strings

GuillaumeDecMeetsMore · 2024-08-15T06:19:16Z

scheduler/clients/javascript/tests/requirements/requirements.spec.ts

@@ -380,12 +380,230 @@ describe('Requirements', () => {
      it.todo('To be implemented')
    })

-    describe("Users' calendars can be queried in groups", () => {
-      it.todo('To be implemented')
+    describe('Multiple calendars of the same user can be queried at once', () => {


Not linked to the new feature of querying on multiple users for freebusy, but worth adding

GuillaumeDecMeetsMore · 2024-08-15T06:19:39Z

scheduler/clients/javascript/tests/requirements/requirements.spec.ts

    })

-    describe("Users' availability can be queried in groups", () => {
-      it.todo('To be implemented')
+    describe('Multiple calendars of different users can be queried at once', () => {


This is freebusy of multiple users

mm-zacharydavison

Satisfying PR! Some questions.

mm-zacharydavison · 2024-08-15T14:04:50Z

scheduler/clients/javascript/lib/userClient.ts

+  userIds: UUID[]
+  /**
+   * Start time of the period to check for freebusy
+   * @format Date in UTC


I assume you've thought of this so maybe just catch me up, but:

my expectation as a user of the client would be that I'd be able to query with timezone information, this seems like the main place someone would make a mistake there.

Is there a reason why this expects UTC?

I have started looking into a bit more in #28, following the article that Derek shared, but it's a bit more complex than expected (I need to invest more time).

Ideally, I would like (at least) the JS SDK, the Rust SDK and the API to accept any timezone.
The API could still only handle only UTC datetimes internally. In the PR above, I've tried to have timezones everywhere (so, never convert to UTC, but it became quite complex so I decided to focus on the other requirements as this one is not blocking per se

Good for me, we might need to revisit the API design later though.

The reason I ask is you're going to immediately have this problem I think since we're operating in Tokyo timezone.

scheduler/clients/javascript/lib/userClient.ts

mm-zacharydavison · 2024-08-15T14:11:14Z

scheduler/crates/api/src/user/get_multiple_users_freebusy.rs

+    }
+}
+
+#[cfg(test)]


Is it idiomatic to put the test in the same file in Rust?

It depends:

it's idiomatic to have unit tests in the same file

it's idiomatic to have integration tests in a specific "tests" directory

It's worth noting that having unit tests in the same file allows to access private functions, structs, etc., of that module (as the tests are effectively inside the module)

(source)

TIL! I like that actually.

mm-zacharydavison · 2024-08-19T08:22:12Z

scheduler/clients/javascript/lib/userClient.ts

+  userIds: UUID[]
+  /**
+   * Start time of the period to check for freebusy
+   * @format Date in UTC


Good for me, we might need to revisit the API design later though.

The reason I ask is you're going to immediately have this problem I think since we're operating in Tokyo timezone.

mm-zacharydavison · 2024-08-19T08:22:37Z

scheduler/clients/javascript/lib/userClient.ts

   */
-  busy: CalendarEventInstance[]
+  [key: string]: CalendarEventInstance[]


[nit] Shouldn't this be key: UUID

@GuillaumeDecMeetsMore minor nit, otherwise LGTM.

mm-zacharydavison · 2024-08-19T08:23:16Z

scheduler/crates/api/src/user/get_multiple_users_freebusy.rs

+    }
+}
+
+#[cfg(test)]


TIL! I like that actually.

GuillaumeDecMeetsMore and others added 30 commits July 11, 2024 11:32

refactor: adapt/update js client

805f4e2

chore: update justfile

941e65f

ci: adapt JS client lib's CI

d2234a7

test: start adding integration tests for our use cases

446f130

ci: fix actions/setup-node usage for pnpm

0a4f16c

ci: update actions + install pnpm

1f0bd04

ci: remove cache on actions/setup-node as it's pretty fast

b1df0f7

Merge branch 'guillaume/ci/adapt-js-client-ci' into guillaume/test/ad…

cee3d0f

…d-integration-test-for-our-use-cases

Merge branch 'master' into guillaume/test/add-integration-test-for-ou…

a08ea16

…r-use-cases

test: add more tests to integration tests

8bac479

Merge branch 'master' into guillaume/test/add-integration-test-for-ou…

10439cb

…r-use-cases

refactor: rename file + adapt requirements' tests

6800201

feat(users): allow to provide external userId (uuid)

e556c34

chore: fix formatting

a08d635

test(requirements): provide userUuid

4626cf1

feat(js lib): add format + lint biome + fixes

6d1bf21

refactor: convert unix timestamps to datetimes

47d560a

Merge branch 'master' into guillaume/feat/user-allow-to-provide-exter…

1e9d5af

…nal-user-uuid

Merge branch 'guillaume/feat/user-allow-to-provide-external-user-uuid…

0e37677

…' into guillaume/feat/add-format-lint-via-biome

Merge branch 'guillaume/feat/add-format-lint-via-biome' into guillaum…

a56b001

…e/refactor/convert-unix-timestamps-to-datetimes

Merge branch 'master' into guillaume/refactor/convert-unix-timestamps…

515f73e

…-to-datetimes

fix: format & lint

8d0f7bd

Merge branch 'master' into guillaume/refactor/convert-unix-timestamps…

dc66f27

…-to-datetimes

Merge branch 'master' into guillaume/refactor/convert-unix-timestamps…

dc6757e

…-to-datetimes

fix: missing imports

f420558

fix: format

11749f3

fix: make client lib return Dates

57a181b

chore: add js docs to client lib

bd43e72

chore: add more JSDocs

8f1bd7f

feat: allow to query multiple calendars + allow to query on name + al…

ee5164a

…low query metadata

GuillaumeDecMeetsMore added 11 commits July 29, 2024 18:18

chore: remove debug logs

c14dfc5

feat: add query freebusy on multiple users + fix metadata

804da1c

refactor: extract convertEventDates & convertInstanceDates

0f55a28

chore: remove comment

3861c55

Merge branch 'guillaume/refactor/convert-unix-timestamps-to-datetimes…

72147a9

…' into guillaume/feat/add-js-docs-client-lib

Merge branch 'master' into guillaume/feat/add-js-docs-client-lib

9bdd463

chore: add more comments

5ff58a4

refactor: rework result of "freebusy of multiple users"

ba6229a

fix: client lib multiple freebusy

7f0b094

Merge branch 'guillaume/feat/add-js-docs-client-lib' into guillaume/f…

af390af

…eat/allow-to-query-free-busy-multiple

chore: remove duplicate requirements

821a881

Base automatically changed from guillaume/feat/add-js-docs-client-lib to master August 1, 2024 09:26

Merge branch 'master' into guillaume/feat/allow-to-query-free-busy-mu…

1b8e859

…ltiple

GuillaumeDecMeetsMore marked this pull request as ready for review August 15, 2024 06:14

GuillaumeDecMeetsMore commented Aug 15, 2024

View reviewed changes

GuillaumeDecMeetsMore changed the title ~~[NO-ISSUE] feat: allow to query multiple calendars + allow to query on name + al…~~ [NO-ISSUE] feat: allow to query multiple calendars Aug 15, 2024

GuillaumeDecMeetsMore changed the title ~~[NO-ISSUE] feat: allow to query multiple calendars~~ [NO-ISSUE] feat: allow to query multiple calendars (freebusy) Aug 15, 2024

GuillaumeDecMeetsMore changed the title ~~[NO-ISSUE] feat: allow to query multiple calendars (freebusy)~~ [NO-ISSUE] feat: allow to query multiple users (freebusy) Aug 15, 2024

GuillaumeDecMeetsMore commented Aug 15, 2024

View reviewed changes

GuillaumeDecMeetsMore requested a review from mm-zacharydavison August 15, 2024 06:20

mm-zacharydavison requested changes Aug 15, 2024

View reviewed changes

refactor: change multipleFreeBusy URL path

9693b49

GuillaumeDecMeetsMore requested a review from mm-zacharydavison August 19, 2024 06:45

mm-zacharydavison approved these changes Aug 19, 2024

View reviewed changes

GuillaumeDecMeetsMore merged commit 8ebfbd1 into master Aug 19, 2024
2 checks passed

GuillaumeDecMeetsMore deleted the guillaume/feat/allow-to-query-free-busy-multiple branch August 19, 2024 09:06

GuillaumeDecMeetsMore mentioned this pull request Aug 19, 2024

[NO-ISSUE] fix: use uuid as key #32

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NO-ISSUE] feat: allow to query multiple users (freebusy) #26

[NO-ISSUE] feat: allow to query multiple users (freebusy) #26

GuillaumeDecMeetsMore commented Jul 29, 2024 •

edited

Loading

GuillaumeDecMeetsMore Aug 15, 2024

GuillaumeDecMeetsMore Aug 15, 2024 •

edited

Loading

GuillaumeDecMeetsMore Aug 15, 2024

GuillaumeDecMeetsMore Aug 15, 2024

GuillaumeDecMeetsMore Aug 15, 2024

mm-zacharydavison left a comment

mm-zacharydavison Aug 15, 2024

GuillaumeDecMeetsMore Aug 19, 2024

mm-zacharydavison Aug 19, 2024

mm-zacharydavison Aug 15, 2024

GuillaumeDecMeetsMore Aug 19, 2024

mm-zacharydavison Aug 19, 2024

mm-zacharydavison Aug 19, 2024

mm-zacharydavison Aug 19, 2024

mm-zacharydavison Aug 19, 2024

mm-zacharydavison Aug 19, 2024

		@@ -1,4 +1,5 @@
		export DATABASE_URL := "postgresql://postgres:postgres@localhost:45432/nettuscheduler"
		export RUST_BACKTRACE := "1"

[NO-ISSUE] feat: allow to query multiple users (freebusy) #26

[NO-ISSUE] feat: allow to query multiple users (freebusy) #26

Conversation

GuillaumeDecMeetsMore commented Jul 29, 2024 • edited Loading

Changed

Choose a reason for hiding this comment

GuillaumeDecMeetsMore Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm-zacharydavison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeDecMeetsMore commented Jul 29, 2024 •

edited

Loading

GuillaumeDecMeetsMore Aug 15, 2024 •

edited

Loading