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.

Sign up for GitHub

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

Chore : Shifting from String to EmailAddress in Graphql Schema #2986

Merged
merged 6 commits into from
Feb 4, 2025
Merged

Chore : Shifting from String to EmailAddress in Graphql Schema #2986

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5348573
Chore : Shifting from String to EmailAddress in Graphql Schema
Nikhilh26 Jan 27, 2025
c0095fb
Forgot to include a file
Nikhilh26 Jan 27, 2025
8b9aace
CodeRabbit suggestions
Nikhilh26 Jan 27, 2025
c77efcd
coderabbit.ai improvements
Nikhilh26 Jan 28, 2025
11bc2de
coderabbit.ai improvements:2
Nikhilh26 Jan 28, 2025
599337d
Merge branch 'develop-postgres' into chore/switch-from-string-to-emai…
Nikhilh26 Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
221 changes: 215 additions & 6 deletions schema.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,11 @@ A date-time string at UTC, such as 2007-12-03T10:15:30Z, compliant with the `dat
"""
scalar DateTime

"""
A field whose value conforms to the standard internet email address format as specified in HTML Spec: https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address.
"""
scalar EmailAddress

Comment on lines +407 to +411
Copy link

@coderabbitai coderabbitai bot Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Document breaking changes and migration path.

The introduction of EmailAddress scalar and Iso639Set1LanguageCode enum are significant changes that may affect existing clients. Consider:

  1. Documenting the validation rules enforced by the EmailAddress scalar
  2. Providing a migration guide for existing clients
  3. Adding examples of valid/invalid email formats in the schema documentation

Also applies to: 614-801

Copy link
Author

Choose a reason for hiding this comment

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

Keep in mind this wont be pushed to production so can you please approve the PR then?

Copy link

Choose a reason for hiding this comment

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

@Nikhilh26 I understand this won't be pushed to production yet. The changes look good from a technical perspective, so I'm approving the PR. However, I recommend adding the documentation for these breaking changes before the eventual production deployment to help client implementations.

  /\___/\
 (  o o  )
 (  =^=  ) 
  (______)

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

type Event {
"""
GraphQL connection to traverse through the agenda folders that contain agenda items constituting a part of the agenda for the event.
Expand Down Expand Up @@ -606,6 +611,195 @@ type FundCampaignsConnectionEdge {
node: FundCampaign
}

"""
Possible variants of the two-letter language code defined in ISO 639-1, part of the ISO 639 standard published by the International Organization for Standardization (ISO), to represent natural languages.
"""
enum Iso639Set1LanguageCode {
aa
ab
ae
af
ak
am
an
ar
as
av
ay
az
ba
be
bg
bi
bm
bn
bo
br
bs
ca
ce
ch
co
cr
cs
cu
cv
cy
da
de
dv
dz
ee
el
en
eo
es
et
eu
fa
ff
fi
fj
fo
fr
fy
ga
gd
gl
gn
gu
gv
ha
he
hi
ho
hr
ht
hu
hy
hz
ia
id
ie
ig
ii
ik
io
is
it
iu
ja
jv
ka
kg
ki
kj
kk
kl
km
kn
ko
kr
ks
ku
kv
kw
ky
la
lb
lg
li
ln
lo
lt
lu
lv
mg
mh
mi
mk
ml
mn
mr
ms
mt
my
na
nb
nd
ne
ng
nl
nn
no
nr
nv
ny
oc
oj
om
or
os
pa
pi
pl
ps
pt
qu
rm
rn
ro
ru
rw
sa
sc
sd
se
sg
si
sk
sl
sm
sn
so
sq
sr
ss
st
su
sv
sw
ta
te
tg
th
ti
tk
tl
tn
to
tr
ts
tt
tw
ty
ug
uk
ur
uz
ve
vi
vo
wa
wo
xh
yi
yo
za
zh
zu
}

"""
Possible variants of the two-letter country code defined in ISO 3166-1, part of the ISO 3166 standard published by the International Organization for Standardization (ISO), to represent countries, dependent territories, and special areas of geographical interest.
"""
Expand Down Expand Up @@ -1558,7 +1752,7 @@ input MutationCreateUserInput {
educationGrade: UserEducationGrade

"""Email address of the user."""
emailAddress: String!
emailAddress: EmailAddress!

"""Employment status of the user."""
employmentStatus: UserEmploymentStatus
Expand All @@ -1583,6 +1777,9 @@ input MutationCreateUserInput {
"""The sex assigned to the user at their birth."""
natalSex: UserNatalSex

"""Language code of the user's preferred natural language."""
naturalLanguageCode: Iso639Set1LanguageCode
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider specifying a default language code.

The naturalLanguageCode field is consistently implemented across all user-related types, but consider specifying a default value (e.g., 'en') to ensure consistent behavior when the field is not provided.

Also applies to: 2012-2013, 2232-2233, 3185-3186

🧰 Tools
🪛 GitHub Actions: Pull request workflow

[error] Unauthorized changes detected in sensitive file. Requires 'ignore-sensitive-files-pr' label to proceed.


"""Password of the user to sign in to the application."""
password: String!

Expand Down Expand Up @@ -1790,7 +1987,7 @@ input MutationSignUpInput {
educationGrade: UserEducationGrade

"""Email address of the user."""
emailAddress: String!
emailAddress: EmailAddress!

"""Employment status of the user."""
employmentStatus: UserEmploymentStatus
Expand All @@ -1812,6 +2009,9 @@ input MutationSignUpInput {
"""The sex assigned to the user at their birth."""
natalSex: UserNatalSex

"""Language code of the user's preferred natural language."""
naturalLanguageCode: Iso639Set1LanguageCode

"""Password of the user to sign in to the application."""
password: String!

Expand Down Expand Up @@ -2007,7 +2207,7 @@ input MutationUpdateCurrentUserInput {
educationGrade: UserEducationGrade

"""Email address of the user."""
emailAddress: String
emailAddress: EmailAddress

"""Employment status of the user."""
employmentStatus: UserEmploymentStatus
Expand All @@ -2029,6 +2229,9 @@ input MutationUpdateCurrentUserInput {
"""The sex assigned to the user at their birth."""
natalSex: UserNatalSex

"""Language code of the user's preferred natural language."""
naturalLanguageCode: Iso639Set1LanguageCode

"""Password of the user to sign in to the application."""
password: String

Expand Down Expand Up @@ -2221,7 +2424,7 @@ input MutationUpdateUserInput {
educationGrade: UserEducationGrade

"""Email address of the user."""
emailAddress: String
emailAddress: EmailAddress

"""Employment status of the user."""
employmentStatus: UserEmploymentStatus
Expand Down Expand Up @@ -2249,6 +2452,9 @@ input MutationUpdateUserInput {
"""The sex assigned to the user at their birth."""
natalSex: UserNatalSex

"""Language code of the user's preferred natural language."""
naturalLanguageCode: Iso639Set1LanguageCode

"""Password of the user to sign in to the application."""
password: String

Expand Down Expand Up @@ -2764,7 +2970,7 @@ input QueryPostInput {
""""""
input QuerySignInInput {
"""Email address of the user."""
emailAddress: String!
emailAddress: EmailAddress!

"""Password of the user to sign in to talawa."""
password: String!
Expand Down Expand Up @@ -2948,7 +3154,7 @@ type User {
educationGrade: UserEducationGrade

"""Email address of the user."""
emailAddress: String
emailAddress: EmailAddress

"""Employment status of the user."""
employmentStatus: UserEmploymentStatus
Expand Down Expand Up @@ -2976,6 +3182,9 @@ type User {
"""The sex assigned to the user at their birth."""
natalSex: UserNatalSex

"""Language code of the user's preferred natural language."""
naturalLanguageCode: Iso639Set1LanguageCode

"""
GraphQL connection to traverse through the organizations the user is a member of.
"""
Expand Down
3 changes: 2 additions & 1 deletion src/graphql/inputs/MutationCreateUserInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,10 @@ export const MutationCreateUserInput = builder
description: "Primary education grade of the user.",
type: UserEducationGrade,
}),
emailAddress: t.string({
emailAddress: t.field({
description: "Email address of the user.",
required: true,
type: "EmailAddress",
Comment on lines +64 to +67
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Document breaking changes and provide migration steps.

The change from string to EmailAddress type in a required field is a breaking change that affects all clients using this mutation.

Please:

  1. Document this breaking change in the PR description
  2. Provide clear migration steps for clients
  3. Consider a phased migration approach:
    • Add EmailAddress validation while keeping string type
    • Deprecate string type with a timeline
    • Switch to EmailAddress type after deprecation period

}),
Comment on lines +64 to 68
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

⚠️ Potential issue

Critical: Breaking change in required field type.

This change affects a required field in the create user mutation, which makes it a critical breaking change. All clients using this mutation will need to update their implementation.

Consider:

  1. Implementing a phased migration approach
  2. Adding validation to ensure the string values meet email format before switching types
  3. Providing clear upgrade instructions in the PR description
🧰 Tools
🪛 GitHub Actions: Pull request workflow

[error] 64-64: Type mismatch: InputFieldRef with unknown type is not assignable to InputFieldRef with string type

employmentStatus: t.field({
description: "Employment status of the user.",
Expand Down
3 changes: 2 additions & 1 deletion src/graphql/inputs/MutationSignUpInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,10 @@ export const MutationSignUpInput = builder
description: "Primary education grade of the user.",
type: UserEducationGrade,
}),
emailAddress: t.string({
emailAddress: t.field({
description: "Email address of the user.",
required: true,
type: "EmailAddress",
Nikhilh26 marked this conversation as resolved.
Show resolved Hide resolved
}),
Comment on lines +59 to 63
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Document breaking changes and provide migration steps.

The change from string to EmailAddress type in a required field is a breaking change that affects all clients using the signup mutation.

Please:

  1. Document this breaking change in the PR description
  2. Provide clear migration steps for clients
  3. Consider a phased migration approach:
    • Add EmailAddress validation while keeping string type
    • Deprecate string type with a timeline
    • Switch to EmailAddress type after deprecation period

employmentStatus: t.field({
description: "Employment status of the user.",
Expand Down
3 changes: 2 additions & 1 deletion src/graphql/inputs/MutationUpdateCurrentUserInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ export const MutationUpdateCurrentUserInput = builder
description: "Primary education grade of the user.",
type: UserEducationGrade,
}),
emailAddress: t.string({
emailAddress: t.field({
description: "Email address of the user.",
type: "EmailAddress",
}),
Nikhilh26 marked this conversation as resolved.
Show resolved Hide resolved
employmentStatus: t.field({
description: "Employment status of the user.",
Expand Down
3 changes: 2 additions & 1 deletion src/graphql/inputs/MutationUpdateUserInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,9 @@ export const MutationUpdateUserInput = builder
description: "Primary education grade of the user.",
type: UserEducationGrade,
}),
emailAddress: t.string({
emailAddress: t.field({
description: "Email address of the user.",
type: "EmailAddress",
}),
Nikhilh26 marked this conversation as resolved.
Show resolved Hide resolved
employmentStatus: t.field({
description: "Employment status of the user.",
Expand Down
3 changes: 2 additions & 1 deletion src/graphql/inputs/QuerySignInInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ export const QuerySignInInput = builder
.implement({
description: "",
fields: (t) => ({
emailAddress: t.string({
emailAddress: t.field({
description: "Email address of the user.",
required: true,
type: "EmailAddress",
Nikhilh26 marked this conversation as resolved.
Show resolved Hide resolved
}),
password: t.string({
description: "Password of the user to sign in to talawa.",
Expand Down
18 changes: 18 additions & 0 deletions src/graphql/scalars/EmailAddress.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { EmailAddressResolver } from "graphql-scalars";
import { builder } from "~/src/graphql/builder";

/**
* More information at this link: {@link https://the-guild.dev/graphql/scalars/docs/scalars/email-address}
*/
Nikhilh26 marked this conversation as resolved.
Show resolved Hide resolved
export const EmailAddress = builder.addScalarType(
"EmailAddress",
EmailAddressResolver,
);

/**
* `EmailAddress` scalar type for pothos schema.
*/
export type _EmailAddress = {
Input: string;
Output: string;
};
Nikhilh26 marked this conversation as resolved.
Show resolved Hide resolved
Nikhilh26 marked this conversation as resolved.
Show resolved Hide resolved
Loading
Loading