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.

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

Dashboard api #81

Merged
merged 18 commits into from
Feb 22, 2024
Merged

Dashboard api #81

merged 18 commits into from
Feb 22, 2024

Conversation

scrummish
Copy link
Contributor

No description provided.

@scrummish scrummish requested review from mbwatson and Woozl January 4, 2024 16:12
Copy link
Member

@Woozl Woozl left a comment

Choose a reason for hiding this comment

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

I think this is a pretty good start at this. There are a couple small notes I made on the code and some questions for clarifications. There are 3 places I noticed are missing or broken that may be fixable on our end or simply limitations of the dashboard API that we'll need Sandy to add:

  1. Project spotlight missing cover images:
    screenshot
    image
  2. Project/Collaboration/Teams page people lists are missing images:
    screenshot image
  3. Project page has descriptions that are no longer truncated. Also this will probably require an API change but the the projects page now takes ~12 seconds to load/hydrate because the people endpoint returns a 10k line/345kb json response.
    screenshot image

@@ -7,7 +7,7 @@ import { useTheme } from "@emotion/react";

export const PersonCard = ({ person, showTitle = false, anchorName }) => {
const theme = useTheme();

console.log(person)
Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -7,6 +7,7 @@ import { useTheme } from '@mui/material/styles'
import { MarkdownLess } from './markdown'

export const ProjectCard = ({project}) => {
console.log(project)
Copy link
Member

Choose a reason for hiding this comment

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

remove

...options,
};
const requestUrl = getDashboardURL(endpoint);
process.env['NODE_TLS_REJECT_UNAUTHORIZED'] = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for this? Is this only happening in dev? Node docs highly discourage its use

From: https://nodejs.org/api/cli.html

If value equals '0', certificate validation is disabled for TLS connections. This makes TLS, and HTTPS by extension, insecure. The use of this environment variable is strongly discouraged.

Copy link
Member

Choose a reason for hiding this comment

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

Dashboard API is over http currently, we will remove this once that has a certificate.

Copy link
Member

Choose a reason for hiding this comment

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

The dashboard is now using SSL at https://dashboard.renci.org so we can remove this

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this in a different branch? Is this supposed to be here again?

@Woozl
Copy link
Member

Woozl commented Jan 5, 2024

Two things I forgot to mention. We discussed yesterday the "our work" menu has collaborations/researchGroups/teams that it's currently still pulling from strapi. Maybe we should migrate this to the dashboard data in this PR as well, if it isn't too much work.

Also, @mbwatson when you do your review, don't forget to be on the VPN, thought I'd mention it since it wasn't and tripped me up for a bit.

@mbwatson
Copy link
Member

mbwatson commented Jan 5, 2024

Also, @mbwatson when you do your review, don't forget to be on the VPN, thought I'd mention it since it wasn't and tripped me up for a bit.

@Woozl thanks for this reminder!

Copy link
Member

@mbwatson mbwatson left a comment

Choose a reason for hiding this comment

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

i see all the same things David noticed (thanks for your thorough inspection!)

we'll want to figure out which we can address and which would be better addressed with an API change.

@mbwatson
Copy link
Member

for us to change, client-side

  • remove some debug logging
  • truncate project descriptions
  • on indiv proj pages & people page, use api/webinfo/people/{id}/photo endpoint URLs instead of base64 images
  • on project list page and indiv proj page, use featuredImage URL
  • remove TLS env var when SSL is complete

API changes to request

  • prefer image URLs to base64-encoding, can remove images from big people list
  • /divisions, or similar, to get Our Work menu lists

@mbwatson mbwatson self-requested a review February 13, 2024 20:32
@Woozl Woozl self-requested a review February 22, 2024 19:15
@scrummish scrummish merged commit 29382e3 into develop Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants