[DCJ-260][risk=no] Converted User.js -> User.ts #2557

sjkobori · 2024-04-29T18:10:41Z

Addresses

https://broadworkbench.atlassian.net/browse/DCJ-260 Typescript Conversion
~~https://broadworkbench.atlassian.net/browse/DCJ-332 Unit tests~~ Moved to #2610

Summary

Converts User.js to TypeScript as well as their corresponding types.
~~Adds Tests for User methods.~~

fboulnois

Looks reasonable minus tests.

src/libs/ajax/User.ts

src/types/model.ts

package.json

src/libs/ajax/User.ts

rushtong · 2024-04-29T20:45:26Z

src/types/model.ts

+  | 'Alumni'
+  | 'SigningOfficial'
+  | 'DataSubmitter'
+  | 'All';


Technically, we don't have a back end role for 'All', but it might be helpful in this context if all means

Any role is allowed to access this resource

src/types/model.ts

aarohinadkarni

looks good besides tests & two possible changes 👍

src/libs/ajax/User.ts

sjkobori · 2024-05-14T09:03:59Z

src/libs/ajax/User.ts

+    // We should not be updating the user's create date, associated institution, or library cards
+    // This below code does not seem to work at all and
+    // does not seem appropriate for this request anyway.
+    // The UpdateDuosUserRequestV2 is not the same shape as a DuosUser
+    // like this flow suggests.
+    const filteredUser = flow(
+      cloneDeep,
+      unset('updatedUser.createDate'),
+      unset('updatedUser.institution'),
+      unset('updatedUser.libraryCards')
+    )(user);


This seems to be a relic of an older implementation of this API. I don't think it is within the scope of this ticket to update/remove this logic, however, I did want to check with the team about this before spinning it off into its own ticket

To be more specific (and I can add this to the comment directly if desired), these unset calls do not in fact unset those properties but the payload being used in the update request does not have those fields anyway.

This is an artifact of the older API. The latest version accepts a limited number of fields that can be updated as per the API docs. I think it is appropriate to leave this logic as is and we can address it in a future ticket.

UserUpdateFields

Current API Docs

sjkobori · 2024-05-14T09:06:47Z

src/libs/ajax/User.ts

+      if (res.ok) {
+        return res.json();
+      }


Wanted to make a callout here. This previously read return res.json without the parentheses and Typescript helped me catch that. There is only one place this method is used and it is being used as a truthy value so I don't believe this should change any behavior.

Regardless, I just wanted to make reviewers aware since it is not clear at all from the diff.

I think this is fine, but it might be redundant if fetchOk fails on a non-OK response. There could be descendant code that also handles the non-OK case in a particular manner, for example, showing a custom error response depending on what res.json() does or does not contain.

Just to clarify - I don't see any issues with moving from res.json -> res.json()

I think ultimately, we should change all these fetchOks to use axios instead. I think it would be wise to have a consistent way of making these requests and if we decide to do any handling of errors, they should happen outside of these ajax calls

rushtong · 2024-05-14T13:19:10Z

src/libs/ajax/User.ts

+    const url = `${await getApiUrl()}/api/user`;
+    // We should not be updating the user's create date, associated institution, or library cards


Note that this is currently broken in dev: https://broadworkbench.atlassian.net/browse/DCJ-362

rushtong

I tested this locally, no regressions. Code looks good, test coverage also looks good 👍🏽 Consider inline comments optional.

rushtong · 2024-05-14T14:15:18Z

cypress/component/unit/ajax/User.spec.ts

+    });
+  });
+
+  //TODO


Is this still relevant?

Nope, missed that one 👍

rushtong · 2024-05-14T14:29:18Z

cypress/component/unit/ajax/User.spec.ts

+          await User.list('Admin');
+        } catch (err) {


Do we need anything before the catch to ensure that we actually did hit the assert in the catch block? This pattern does test as expected, so I'm fine with this as is.

That's a good point, I'll put some assert(false)s after the ajax calls before the catch block to make sure it fails if it does not throw for some reason

fboulnois

Approved after tests pass.

Revert "Synced package-lock.json" This reverts commit c4059ec. Added unedited package-lock.json Revert "Added unedited package-lock.json" This reverts commit 8aef944. Manual sync of package-lock.json Fixed some typos from manual correction

Converted User.js -> User.ts

099b75f

sjkobori requested a review from a team as a code owner April 29, 2024 18:10

sjkobori requested review from fboulnois and aarohinadkarni and removed request for a team April 29, 2024 18:10

fboulnois reviewed Apr 29, 2024

View reviewed changes

src/libs/ajax/User.ts Show resolved Hide resolved

src/types/model.ts Show resolved Hide resolved

Updated the linter to support typescript and resolved tsconfig paths

730a4fd

rushtong reviewed Apr 29, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

rushtong reviewed Apr 29, 2024

View reviewed changes

src/libs/ajax/User.ts Outdated Show resolved Hide resolved

rushtong reviewed Apr 29, 2024

View reviewed changes

src/types/model.ts Outdated Show resolved Hide resolved

aarohinadkarni reviewed Apr 30, 2024

View reviewed changes

src/libs/ajax/User.ts Outdated Show resolved Hide resolved

src/libs/ajax/User.ts Outdated Show resolved Hide resolved

sjkobori added 3 commits May 7, 2024 15:55

Fixed User functions that returned a list to represent that correctly

2602f8a

Unit tests for api/user/me

c974009

User.spec.ts completed

bc5facf

sjkobori changed the title ~~[DCJ-260] Converted User.js -> User.ts~~ [DCJ-260][DCJ-332][risk=no] Converted User.js -> User.ts May 14, 2024

Added DataUse type

8d58f26

sjkobori mentioned this pull request May 14, 2024

[DCJ-346][risk=no] Updated linter to support typescript linting #2585

Merged

sjkobori commented May 14, 2024

View reviewed changes

rushtong reviewed May 14, 2024

View reviewed changes

rushtong approved these changes May 14, 2024

View reviewed changes

Added explicit test failures if a rejection is handled

1706c68

sjkobori force-pushed the DCJ-260-duos-ui-User-module-to-typescript branch 2 times, most recently from fb56b8d to 1706c68 Compare May 14, 2024 22:26

Resolved merge

c4151cf

sjkobori force-pushed the DCJ-260-duos-ui-User-module-to-typescript branch from d6b0951 to c4151cf Compare May 15, 2024 01:49

sjkobori added 2 commits May 15, 2024 10:34

Moved cy methods to outside of async tests, mocked getApiUrl

cb6fea4

Resolved merge conflict

c1e0c21

fboulnois approved these changes May 17, 2024

View reviewed changes

sjkobori mentioned this pull request Jun 25, 2024

[DCJ-332] user.ts unit tests #2610

Closed

Removed User unit tests (moved to standalone PR

a7e6e2f

sjkobori changed the title ~~[DCJ-260][DCJ-332][risk=no] Converted User.js -> User.ts~~ [DCJ-260][risk=no] Converted User.js -> User.ts Jun 25, 2024

sjkobori marked this pull request as draft June 27, 2024 20:00

sjkobori marked this pull request as ready for review June 27, 2024 20:00

Synced package-lock.json

270a020

Revert "Synced package-lock.json" This reverts commit c4059ec. Added unedited package-lock.json Revert "Added unedited package-lock.json" This reverts commit 8aef944. Manual sync of package-lock.json Fixed some typos from manual correction

sjkobori force-pushed the DCJ-260-duos-ui-User-module-to-typescript branch from 9a7be80 to 270a020 Compare July 1, 2024 02:42

sjkobori merged commit 2159399 into develop Jul 1, 2024
9 checks passed

sjkobori deleted the DCJ-260-duos-ui-User-module-to-typescript branch July 1, 2024 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DCJ-260][risk=no] Converted User.js -> User.ts #2557

[DCJ-260][risk=no] Converted User.js -> User.ts #2557

sjkobori commented Apr 29, 2024 •

edited

Loading

fboulnois left a comment

rushtong Apr 29, 2024

aarohinadkarni left a comment

sjkobori May 14, 2024

sjkobori May 14, 2024

rushtong May 14, 2024

sjkobori May 14, 2024 •

edited

Loading

rushtong May 14, 2024

rushtong May 14, 2024

sjkobori May 14, 2024

rushtong May 14, 2024

rushtong left a comment

rushtong May 14, 2024

sjkobori May 14, 2024

rushtong May 14, 2024

sjkobori May 14, 2024

fboulnois left a comment

		const url = `${await getApiUrl()}/api/user`;
		// We should not be updating the user's create date, associated institution, or library cards

+                  });
+                });
+                //TODO

[DCJ-260][risk=no] Converted User.js -> User.ts #2557

[DCJ-260][risk=no] Converted User.js -> User.ts #2557

Conversation

sjkobori commented Apr 29, 2024 • edited Loading

Addresses

Summary

fboulnois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarohinadkarni 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

sjkobori May 14, 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

Choose a reason for hiding this comment

rushtong 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

fboulnois left a comment

Choose a reason for hiding this comment

sjkobori commented Apr 29, 2024 •

edited

Loading

sjkobori May 14, 2024 •

edited

Loading