[Issue #3308] View Saved Opportunities on Saved grants page #3992
Conversation
acouch
requested review from
andycochran,
btabaska and
doug-s-nava
as code owners
acouch
force-pushed
the
acouch/issue-3308-view-saved-opps
branch
from
39a096b to
7113ea5
Compare
There was a problem hiding this comment.
a couple of nit picks and one edge case that I'm not sure we want to cover here or handle later.
If you're viewing a search result that contains a saved opportunity, then you log out, the opportunity still shows up as saved. It gets removed on refresh, so maybe we want to just refresh the page on logout?
| SavedOpportunity[] | ||
| > => { | ||
| try { | ||
| const session = await getSession(); |
There was a problem hiding this comment.
It may be overkill, but since the session will only be available on the server side (until now it's only been used in route handlers) maybe we should put a "server only" on this file?
There was a problem hiding this comment.
The client component would immediately break with a notice if you tried to import the file, so doesn't seem like a risk that someone would use this incorrectly. server-only designates it is a server function, but also that it should be able to be used directly by the client, which isn't true in this case. Do we want to add server-only to every file that is server side?
There was a problem hiding this comment.
Agree there's no big risk here since this would break if we tried to use it in the client, but potentially would add some dev time if someone didn't realize that. If we pop "server-only" in here, since it's a utilities file rather than anything react specific, I think we'd gain the ability to break faster and provide an annotation for devs so that the use case for the file is immediately clear. https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#keeping-server-only-code-out-of-the-client-environment
Not entirely necessary, but worth maybe discussing further and thinking about documenting as we make a decision one way or another about how we want to use these designations.
There was a problem hiding this comment.
I was conflating server-only and use server, the former poisoning clients from importing while the latter endorsing. I added server-only.
| return ( | ||
| <ul className="usa-prose usa-list--unstyled"> | ||
| {opportunities.map((opportunity) => ( | ||
| <li key={opportunity?.opportunity_id}> |
There was a problem hiding this comment.
doesn't seem like this would probably be possible, but if we could have undefined opportunities coming through that could mean non-unique keys here. We could either just assert that the opportunity will always be defined or add a unique fallback value?
There was a problem hiding this comment.
I don't like the idea of validating things like that in a client component, and instead enforce that on the data fetching side. I updated it for this case.
| export default async function SavedGrants({ params }: LocalizedPageProps) { | ||
| const { locale } = await params; | ||
| const t = await getTranslations({ locale }); | ||
| const savedOpportunities = await fetchSavedOpportunities(); |
There was a problem hiding this comment.
[a note for later] is there a way we could move this complexity to the API side so that we don't have to make so many round trips here, or will caching make that more or less a moot point?
There was a problem hiding this comment.
It would be ideal if the saved opportunity returned enough detail to create the results summary. Since that is changing with the new design we shouldn't make a change there right now.
| export default async function SavedGrants({ params }: LocalizedPageProps) { | ||
| const { locale } = await params; | ||
| const t = await getTranslations({ locale }); | ||
| const savedOpportunities = await fetchSavedOpportunities(); | ||
| const opportunities = savedOpportunities.map(async (savedOpportunity) => { |
There was a problem hiding this comment.
something like "opportunityDetailPromises" would be clearer to me here
There was a problem hiding this comment.
The getOpportunityDetails returns:
interface OpportunityApiResponse extends APIResponse {
data: Opportunity;
}For me it makes sense to describe the type of data we are returning, vs the function used to retrieve it. Does opportuniyPromises make more sense?
There was a problem hiding this comment.
tldr; yep, opportunityPromises is great.
I'm willing to go either way, since the promise piece of this is in the type, and explaining the type in the variable name is also generally a no-no, but in cases like this where a synchronous map function returns the promises, and then you await them later, I find it's often confusing at what point we're dealing with unresolved promises and at what point we're dealing with actual data. Somehow making it clear that this variable doesn't yet represent opportunities, but will in the future, seems helpful to me.
There was a problem hiding this comment.
I updated this and added docs so we are clear in the future.
|
I created a separate ticket for the logout #4022 |
Summary
Fixes #3308
Time to review: 15 mins
Changes proposed
Add saved opps to Saved grants page:
Desktop
Mobile
Desktop (no saved opps, no change)
Mobile (no saved opps, no change)
Context for reviewers
To test: