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

lib-user: Refactor usePanoptesProjects hook with auth, update usage accordingly #6140

Merged
merged 5 commits into from
Jun 19, 2024
Merged

lib-user: Refactor usePanoptesProjects hook with auth, update usage accordingly #6140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c77e861
Refactor usePanoptesProjects with auth and query param
mcbouslog Jun 19, 2024
e8a378e
Refactor RecentProjectsContainer and RecentSubjectsContainer per useP…
mcbouslog Jun 19, 2024
6c67fb3
Refactor UserStats and Certificate per usePanoptesProjects changes
mcbouslog Jun 19, 2024
09642fc
Refactor GroupStats and Contributors per usePanoptesProjects changes
mcbouslog Jun 19, 2024
9fcf48a
Refactor auth header to undefined if no auth
mcbouslog Jun 19, 2024
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
7 changes: 5 additions & 2 deletions packages/lib-user/src/components/Certificate/CertificateContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,15 @@ function CertificateContainer({
})

// fetch projects, if selectedProject is not 'AllProjects'
const projectIds = selectedProject !== 'AllProjects' ? [selectedProject] : null
const projectId = selectedProject !== 'AllProjects' ? selectedProject : null
const {
data: projects,
error: projectsError,
isLoading: projectsLoading
} = usePanoptesProjects(projectIds)
} = usePanoptesProjects({
cards: true,
id: projectId
})

const hours = convertStatsSecondsToHours(stats?.time_spent)
const projectsCount = stats?.project_contributions?.length || 0
Expand Down
6 changes: 5 additions & 1 deletion packages/lib-user/src/components/Contributors/Contributors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,11 @@ function Contributors({
data: projects,
error: projectsError,
isLoading: projectsLoading
} = usePanoptesProjects(projectIds)
} = usePanoptesProjects({
cards: true,
id: projectIds?.join(','),
page_size: 100
})

// combine member stats with user data
let contributors = []
Expand Down
8 changes: 6 additions & 2 deletions packages/lib-user/src/components/GroupStats/GroupStats.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,17 @@ function GroupStats({
})

// fetch projects
const projectIDs = allProjectsStats?.project_contributions?.map(project => project.project_id)
const projectIds = allProjectsStats?.project_contributions?.map(project => project.project_id)

const {
data: projects,
error: projectsError,
isLoading: projectsLoading
} = usePanoptesProjects(projectIDs)
} = usePanoptesProjects({
cards: true,
id: projectIds?.join(','),
page_size: 100
})

function handleGroupModalActive () {
setGroupModalActive(!groupModalActive)
Expand Down
36 changes: 21 additions & 15 deletions ...ges/lib-user/src/components/UserHome/components/RecentProjects/RecentProjectsContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ async function fetchUserProjectPreferences() {
const authorization = `Bearer ${token}`
try {
const query = {
page: 1, // returns 20 items
sort: '-updated_at',
user_id: user.id
}
Expand All @@ -29,7 +28,6 @@ async function fetchUserProjectPreferences() {
const projectPreferencesUserHasClassified =
response.body.project_preferences
.filter(preference => preference.activity_count > 0)
.slice(0, 10)
return projectPreferencesUserHasClassified
}
return []
Expand All @@ -39,7 +37,7 @@ async function fetchUserProjectPreferences() {
}
}

export default function RecentProjectsContainer({ authUser }) {
function RecentProjectsContainer({ authUser }) {
// Get user's project preference.activity_count for 10 most recently classified projects
const cacheKey = {
name: 'user-project-preferences',
Expand All @@ -59,21 +57,27 @@ export default function RecentProjectsContainer({ authUser }) {
data: projects,
isLoading: projectsLoading,
error: projectsError
} = usePanoptesProjects(recentProjectIds)

} = usePanoptesProjects({
cards: true,
id: recentProjectIds?.join(',')
})

// Attach project object to each project preference
let projectPreferencesWithProjectObj

if (projects?.length) {
projectPreferencesWithProjectObj = projectPreferences?.map(preference => {
const matchedProjectObj = projects.find(
project => project.id === preference.links.project
)
projectPreferencesWithProjectObj = projectPreferences
.map(preference => {
const matchedProjectObj = projects.find(
project => project.id === preference.links?.project
)

if (matchedProjectObj) {
preference.project = matchedProjectObj
}
return preference
})
if (matchedProjectObj) {
preference.project = matchedProjectObj
}
return preference
})
Comment on lines +74 to +78
Copy link
Member

Choose a reason for hiding this comment

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

I think preference.project = whatever will actually modify the original object(s) in the projectPreferences[] array (aka the data fetched via fetchUserProjectPreferences()).

In practice, this is fine because I don't see anything else using projectPreferences, nor for that matter the response.body.project_preferences data being pulled in RecentProjectsContainer.fetchUserProjectPreferences().

⚠️ In theory, this is inadvisable since we've got a component that may actually be modifying the input values being passed to it. That's a bit blargh!

Recommendation: make a copy of the object you want to modify and return. e.g.:

let copyOfPreference = { ...preference }  // Assumes preference is never undefined/null
if (matchedProjectObj) {
  copyOfPreference.project = matchedProjectObj
}
return copyOfPreference

.filter(preference => preference?.project?.slug)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This filter will help prevent similar crashes.

.slice(0, 10)
}

return (
Expand All @@ -90,3 +94,5 @@ RecentProjectsContainer.propTypes = {
id: string
})
}

export default RecentProjectsContainer
32 changes: 23 additions & 9 deletions ...ges/lib-user/src/components/UserHome/components/RecentSubjects/RecentSubjectsContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const SWROptions = {
async function fetchUserRecents({ userId }) {
try {
const query = {
page_size: 10,
sort: '-created_at'
}
const response = await panoptes.get(`/users/${userId}/recents`, query)
Expand All @@ -36,16 +35,31 @@ function RecentSubjectsContainer({ authUser }) {

const recentProjectIds = [...new Set(recents?.map(recent => recent.links?.project))]

const { data: projects, isLoading: projectsLoading, error: projectsError } = usePanoptesProjects(recentProjectIds)
const {
data: projects,
isLoading: projectsLoading,
error: projectsError
} = usePanoptesProjects({
cards: true,
id: recentProjectIds?.join(',')
})

// Attach project slug to each recent
let recentsWithProjectSlugs
if (projects?.length) {
recentsWithProjectSlugs = recents.map(recent => {
const { slug } = projects.find(project => recent.links.project === project.id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shaunanoordin - this was the cause of the page crash. The recents included a project that was not included in the usePanoptesProjects projects, and when this line attempted to get the slug from nothing it caused the crash.

recent.project_slug = slug
return recent
})
recentsWithProjectSlugs = recents
.map(recent => {
const matchedProjectObj = projects.find(
project => project.id === recent.links?.project
)

if (matchedProjectObj) {
recent.projectSlug = matchedProjectObj.slug
}
return recent
Comment on lines +56 to +59
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Similar to my note about modifying the original object(s) in RecentProjectContainer's projectPreferences[], I think this recent.projectSlug = whatever might actually be modifying the original project(s) data.

I'm now curious to see if any other component that uses usePanoptesProject()'s data value is also suddenly seeing data[x].projectSlug 🤔

Similar suggestion applies: create a copy of the project before modifying and returning it.

const copiedRecent = { ...recent }  // Assumes recent isn't null or undefined
if (matchedProjectObj) {
  copiedRecent.projectSlug = matchedProjectObj.slug
}
return copiedRecent

})
.filter(recent => recent?.projectSlug)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This filter will help prevent similar crashes.

.slice(0, 10)
}

const error = recentsError || projectsError
Expand All @@ -60,10 +74,10 @@ function RecentSubjectsContainer({ authUser }) {
)
}

export default RecentSubjectsContainer

RecentSubjectsContainer.propTypes = {
authUser: shape({
id: string
})
}

export default RecentSubjectsContainer
8 changes: 6 additions & 2 deletions packages/lib-user/src/components/UserStats/UserStatsContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,17 @@ function UserStatsContainer({
})

// fetch projects
const projectIDs = allProjectsStats?.project_contributions?.map(project => project.project_id)
const projectIds = allProjectsStats?.project_contributions?.map(project => project.project_id)

const {
data: projects,
error: projectsError,
isLoading: projectsLoading
} = usePanoptesProjects(projectIDs)
} = usePanoptesProjects({
cards: true,
id: projectIds?.join(','),
page_size: 100
})

function handleDateRangeSelect (dateRange) {
setSelectedDateRange(dateRange.value)
Expand Down
37 changes: 25 additions & 12 deletions packages/lib-user/src/hooks/usePanoptesProjects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { projects as panoptesProjects } from '@zooniverse/panoptes-js'
import auth from 'panoptes-client/lib/auth'
import useSWR from 'swr'

const isBrowser = typeof window !== 'undefined'

const SWROptions = {
revalidateIfStale: true,
revalidateOnMount: true,
Expand All @@ -9,17 +12,28 @@ const SWROptions = {
refreshInterval: 0
}

async function fetchProjects(id) {
if (isBrowser) {
auth.checkCurrent()
}
Comment on lines +15 to +17
Copy link
Member

Choose a reason for hiding this comment

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

[curiosity] This is the first time I'm seeing this pattern - auth.checkCurrent() is placed in the root function? Is this synchronous and will this block execution of the rest of the code, until auth current is checked? Or it is running asynchronously as the rest of the page executes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is asynchronous and won't block other code execution. It's similar to what is implemented in lib-react-components' useUnreadMessages and useUnreadNotifications hooks.


async function fetchProjects(query) {
let token = await auth.checkBearerToken()
if (!token) {
await auth.checkCurrent()
token = await auth.checkBearerToken()
}
const authorization = token ? `Bearer ${token}` : undefined

let projectsAccumulator = []

async function getProjects (page = 1) {
const query = {
cards: true,
id,
page,
page_size: 100
}
const response = await panoptesProjects.get({ query })
const response = await panoptesProjects.get({
query: {
page,
...query
},
authorization
Copy link
Member

Choose a reason for hiding this comment

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

[observation] Ah, so what was missing previously was the Authorization, which meant non-public projects wouldn't be fetched. Gotcha.

Copy link
Member

Choose a reason for hiding this comment

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

⚠️ ❓ Wait, hang on - does this mean that usePanoptesProjects() is meant to only work when a user is logged in?

Because if there's no user, the token var above will be blank or undefined, which means that panoptesProjects.get() will be getting a parameter that looks like { authorization: 'Bearer )' } or { authorization: 'Bearer undefined)' }

If usePanoptesProjects() is meant to be used with logged-in users only, we need to add some documentation to alert people of the gotcha. Maybe add in a if (!token) throw new Error('usePanoptesProjects() requires a logged-in user')

If usePanoptesProjects() is meant to be used regardless of logged-in state, then perhaps something needs to be changed. Suggestion: const authorization = (token) ? Bearer ${token} : undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored as noted - const authorization = (token) ? Bearer ${token} : undefined

})
const { meta, projects } = response?.body || {}

if (meta?.projects?.page_count > 5) {
Expand All @@ -42,11 +56,10 @@ async function fetchProjects(id) {
return projectsAccumulator
}

export function usePanoptesProjects(projectIds) {
export function usePanoptesProjects(query) {
let key = null
if (projectIds) {
const id = projectIds.join(',')
key = id
if (query?.id) {
key = query
}
return useSWR(key, fetchProjects, SWROptions)
}