upgrade apollo #2330

lperson · 2023-12-22T15:37:44Z

Fixes #2329

Description

Upgrade apollo components. I needed to upgrade the server components as well because they coexist in package.json.

Checklist:

I have manually tested my changes on desktop and mobile
The test suite passes locally with my changes
If my change is a UI change, I have attached a screenshot to the description section of this pull request
My change is 300 lines of code or less, or has a documented reason in the description why it’s longer
I have made any necessary changes to the documentation
I have added tests that prove my fix is effective or that my feature works
My PR is labeled [WIP] if it is in progress

Updates the versions in `package.json` so they install with no warnings about peer dependencies.

!! OPEN ITEMS !! * Restore GraphQLError handling

lperson · 2023-12-23T17:56:48Z

src/network/apollo-client-singleton.js

@@ -1,10 +1,12 @@
+import {


In addition to my changing the imports VS code rearranged them.

lperson · 2023-12-23T17:57:17Z

src/server/api/mutations/releaseContacts.js

@@ -1,5 +1,4 @@
-import { log } from "../../../lib";
-import { Assignment, r, cacheableData } from "../../models";
+import { r, cacheableData } from "../../models";


deleted unused imports

lperson · 2023-12-24T16:46:37Z

__test__/server/api/campaign/campaign.test.js

@@ -363,7 +363,7 @@ it("should save campaign canned responses across copies and match saved data", a
    testAdminUser
  );
  expect(campaignDataResults.data.campaign.cannedResponses.length).toEqual(6);
-  for (let i = 0; i < 6; i++) {
+  for (let i = 0; i < 6; i += 1) {


The linter complained about i++ so I changed it to i += 1 in several places

lperson · 2023-12-24T16:47:13Z

__test__/server/api/campaign/campaign.test.js

-          assignmentId
-        });
+
+      const waitFor = [];


The linter complained about using await inside a loop so I changed it to use this pattern.

That was a good call. Can you tell me how you decided that it should be an empty array?

I'm not sure I understand the question?

The idea about the linter complaint and the change was to take await out of the loop. We use an array to pass promises to Promise.all() after the loop. We need to start with an empty array to add the promises to.

The whole thing probably could have been done differently, maybe something like the following (as someone did elsewhere in the file)

Promise.all(testContacts.slice(0,5).map(contact => sendMessage(contact.id, testTexterUser, { userId: testTexterUser.id, contactNumber: contact.cell, text: "test text", assignmentId: assignmentId.toString() }))

* Changed arguments to `graphql` to match new signature and in some cases renamed variables to make it more convenient to use them in objects because of shorthand. * Apollo client 3 is stricter about typing so I added some type coersion in the tests * Cleaned some linter warnings about using `await` in loops and using the ++ operator refer to comments in the PR for callouts about some changes

lperson · 2023-12-24T16:52:38Z

__test__/test_helpers.js

-    mySchema,
-    operationText,
+  const contextValue = getContext({ user });
+  const result = await graphql({


Changed arguments to match new signature.

What is the new signature in reference to?

the graphql function takes different parameters in the updated version compared to the older version

…hoose a contact

* Updated parameters for new signature of `graphql` * Coerced types for stricter graphql type checking * Cleaned up lints

…ew signature

lperson · 2023-12-25T00:00:14Z

src/server/api/errors.js

-    throw new GraphQLError({
-      status: 401,
-      message: "You must login to access that resource."
+    throw new GraphQLError("You must login to access that resource.", {


Updated for new signature.

…dFromObject)

Arique1104 · 2023-12-28T16:38:31Z

__test__/extensions/message-handlers/profanity-tagger.test.js

@@ -166,12 +178,15 @@ describe("Message Hanlder: profanity-tagger", () => {
    expect(user).toBe(true);

    // Confirm texter no-match
-    await sendMessage(c.testContacts[1].id, c.testTexterUser, {
-      userId: c.testTexterUser.id,
+    await sendMessage(c.testContacts[1].id.toString(), c.testTexterUser, {


Curious question @lperson

Should we change the data type within the test or within the code?

Or at least, could you help me understand why we would stringify the data result instead of ensuring that the data coming in from testContacts[1].id is already a string?

That would ensure that the source is the correct data type, and instead we add an expect clause that confirms it is a string?

GREAT question!

I think we're ok here. In the client code we're getting all the values via graphql interfaces and they will come in as strings. In the tests we're taking integers from the database (the IDs) and passing them to graphql queries and mutations.

I don't think it's a good idea to change the type in the interface in order to avoid calling toString() and parseInt() in tests.

My reasoning when I made these changes was the UI code is internally consistent and the server code is internally consistent.

However I'm keeping an open mind in case my assumptions fall apart when I start banging on the UI (which is now).

Arique1104 · 2023-12-28T16:47:55Z

src/server/api/phone.js

@@ -16,12 +16,14 @@ export const GraphQLPhone = new GraphQLScalarType({
    if (ast.kind !== Kind.STRING) {
      throw new GraphQLError(
        `Query error: Can only parse strings got a: ${ast.kind}`,
-        [ast]
+        { nodes: [ast] }


What does nodes: [ast] do?

Is this a new dictionary process within graphql? With a data-type change this significant, I'm just curious how you knew this line needed to be updated. And how you knew this was the correct format?

A combination of https://www.apollographql.com/docs/apollo-server/data/errors/ and https://graphql-kr.github.io/graphql-js/error/

This code needs another pass though, which I'll do in a later commit.

nodes: [ast] passes the abstract syntax tree for the graphql query into the error object. The particular code you highlighted looks at the ast and decides if something passed as a phone number looks like a phone number and raises an exception if not. This is happening in the parseLiteral hook which gives us a way to inspect and possibly alter a literal during parsing. GraphQLPhone is a custom type in our code to handle phone numbers.

Interesting! So I checked out the custom type hook you mentioned.

Confirming this is the correct file path: src/server/api/phone.js

The above file is imported to as GraphQLPhone inside our schema src/server/api/schema.js and then exported within the constant resolvers as a value for Phone.

I'm looking for the functionality where you say we could "possibly alter a literal." And now I see that the parseLiteral hook is being used on line 15 of the phone.js file.

Would this functionality get in the way of seeing a toll-free number or a short-code?

I do not know. This is not new functionality. It might be worth entering a ticket to test that and possibly fix it.

For contacts, it's unlikely we'll be using a toll-free number or short code to reach them. This code would be problematic if was used for the phone numbers we use to originate the texts, such as in the number purchasing code (as one example).

also some formatting from my editor

lperson · 2023-12-31T15:14:08Z

src/components/AssignmentTexter/Controls.jsx

@@ -509,8 +511,8 @@ export class AssignmentTexterContactControls extends React.Component {

    const otherResponsesLink =
      currentInteractionStep &&
-        currentInteractionStep.question.filteredAnswerOptions.length > 6 &&
-        filteredCannedResponses.length ? (
+      currentInteractionStep.question.filteredAnswerOptions.length > 6 &&


From here down in this file all the changes are whitespace changes because my editor autoformatted the code.

lperson · 2023-12-31T15:14:51Z

src/components/AssignmentTexter/Controls.jsx

@@ -125,7 +125,9 @@ export class AssignmentTexterContactControls extends React.Component {
    let currentInteractionStep = null;

    if (availableSteps.length > 0) {
-      currentInteractionStep = availableSteps[availableSteps.length - 1];
+      currentInteractionStep = structuredClone(


Do a deep copy of data that originated in props to avoid modifying props

lperson · 2023-12-31T15:15:18Z

src/lib/permissions.js

@@ -13,8 +13,10 @@ export const isRoleGreater = (role1, role2) =>
 export const hasRoleAtLeast = (hasRole, wantsRole) =>
  ROLE_HIERARCHY.indexOf(hasRole) >= ROLE_HIERARCHY.indexOf(wantsRole);

-export const getHighestRole = roles =>
-  roles.sort(isRoleGreater)[roles.length - 1];
+export const getHighestRole = roles => {


make a copy so we don't copy something that came from props

It looks like you added tempRoles. Is this solely to remove an error message within development? Or is there a larger function here that's being updated?

tempRoles is is a variable to hold a deep copy of roles.

Practical reason: with the updates, the code barfs when we try to modify a parameter of a react component. And by barf I mean it's a runtime error.

Best practice: don't modify your function's parameters!

also some formatting from my editor

src/server/api/mutations/updateServiceManager.js

Arique1104 · 2024-01-07T18:09:45Z

src/server/api/mutations/updateServiceManager.js

@@ -1,4 +1,3 @@
-import { GraphQLError } from "graphql/error";
 import { getConfig } from "../../../server/api/lib/config";


It looks like getConfig isn't being used in this file either. Should that be deleted also?

Yes, done! Good catch!

There are lots of places like this to clean up -- unused imports and other lints. I'm interested in doing that or maybe someone else could do it. I'm using vscode with integrated linting and I like to look at clean files (personal preference) but it's also a good practice to have zero lints.

schuyler1d · 2024-01-07T19:32:54Z

I have tested this branch and haven't found any regressions

lperson marked this pull request as draft December 22, 2023 15:37

lperson added the S-work in progress (WIP) Status: PR label for work that is not yet ready to be reviewed label Dec 22, 2023

Upgrade apollo components to latest

c61231e

Updates the versions in `package.json` so they install with no warnings about peer dependencies.

lperson force-pushed the 2329-upgrade-apollo branch from 5c4a158 to c61231e Compare December 22, 2023 15:44

lperson added 4 commits December 22, 2023 10:58

2329: added cors package per apollo upgrade instructions

dc1232f

2329: update code to work with Apollo server 4

994b48b

!! OPEN ITEMS !! * Restore GraphQLError handling

2339 use gql from @apollo-client

86791c6

2339 update to use new apollo client 3 imports

22b0de8

lperson commented Dec 23, 2023

View reviewed changes

lperson added 2 commits December 23, 2023 13:17

2329 bring tests into the new apollo world

001d426

2329 this caused an error which said to make this change

468e96d

lperson commented Dec 24, 2023

View reviewed changes

lperson force-pushed the 2329-upgrade-apollo branch from 396bd78 to 0ef104d Compare December 24, 2023 16:51

lperson commented Dec 24, 2023

View reviewed changes

lperson added 3 commits December 24, 2023 16:36

2329 Make tests pass: convert to string, use min ID, not index 0 to c…

8c32979

…hoose a contact

2329 More tests pass

e6a46b0

* Updated parameters for new signature of `graphql` * Coerced types for stricter graphql type checking * Cleaned up lints

2329 Updated test for authRequired error; updated GraphQLERrror for n…

039e484

…ew signature

lperson commented Dec 25, 2023

View reviewed changes

lperson added 9 commits December 24, 2023 19:20

2329 use new signature for GraphQLError

57b2488

2329 more tests pass

0a29094

2329 await promise.all

e4ee23f

2329 more tests pass

0f57230

2329 all tests pass now!

407b6a4

2329 make tests less flakey

64f4e54

2329 make order of created contacts determinate

f6329d6

2329 splice was not necessary

d11143b

2329 await the save

a3a404f

lperson added 6 commits December 28, 2023 10:56

2329 the linter complains when there is react in a .js file

568ff1a

2329 call await in the loop (restore previous)

68a5859

2329 no graphql landing page in production

5dbc732

2329 make cache work like it did in apollo v2. (also refactored dataI…

23e0366

…dFromObject)

2329 need async to use await; also cleaned up the lints

6641b39

2329 fix import for disabled landing page plugin

a625e05

Arique1104 reviewed Dec 28, 2023

View reviewed changes

lperson added 3 commits December 29, 2023 12:07

2329 refactor some promise loops

d9916a9

2329 forgot to await

754d22c

2329 don't modify props

543e6ec

also some formatting from my editor

lperson commented Dec 31, 2023

View reviewed changes

lperson added 4 commits December 31, 2023 10:16

2329 createMuiTheme changed to createTheme

7188c65

also some formatting from my editor

2329 remove unused imports

dc886e9

2329 bring server exceptions into the new apollo world

eb58abe

2329 structredClone is not available in older versions

6ee9212

Arique1104 reviewed Jan 7, 2024

View reviewed changes

src/server/api/mutations/updateServiceManager.js Show resolved Hide resolved

Arique1104 reviewed Jan 7, 2024

View reviewed changes

2239 delete unused import

2ffe10c

electiondaze marked this pull request as ready for review February 15, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade apollo #2330

upgrade apollo #2330

lperson commented Dec 22, 2023

lperson Dec 23, 2023

lperson Dec 23, 2023

lperson Dec 24, 2023

lperson Dec 24, 2023

Arique1104 Dec 29, 2023

lperson Dec 29, 2023 •

edited

lperson Dec 24, 2023

Arique1104 Dec 29, 2023

lperson Dec 29, 2023

lperson Dec 25, 2023

Arique1104 Dec 28, 2023

lperson Dec 28, 2023

Arique1104 Dec 28, 2023

lperson Dec 28, 2023

lperson Dec 28, 2023

Arique1104 Jan 7, 2024

lperson Jan 7, 2024

lperson Dec 31, 2023

lperson Dec 31, 2023

lperson Dec 31, 2023

Arique1104 Jan 7, 2024

lperson Jan 7, 2024 •

edited

Arique1104 Jan 7, 2024

lperson Jan 7, 2024

lperson Jan 7, 2024

schuyler1d commented Jan 7, 2024

		@@ -1,4 +1,3 @@
		import { GraphQLError } from "graphql/error";
		import { getConfig } from "../../../server/api/lib/config";

upgrade apollo #2330

Are you sure you want to change the base?

upgrade apollo #2330

Conversation

lperson commented Dec 22, 2023

Fixes #2329

Description

Checklist:

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

lperson Dec 29, 2023 • edited

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

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

lperson Jan 7, 2024 • edited

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

schuyler1d commented Jan 7, 2024

lperson Dec 29, 2023 •

edited

lperson Jan 7, 2024 •

edited