lib-user: Add MainContent section

[mcbouslog]

Package

• lib-user

Describe your changes

• refactors first content section with profile header, bar chart, and related components to MainContent component
• adds getCompleteData helper to get complete time series data (including empties) from stats time series data
• refactors BarChart and getDateRangeLabel
• add usePanoptesProjects hook for requesting projects data

How to Review

Helpful explanations that will make your reviewer happy:

• What Zooniverse project should my reviewer use to review UX? n/a
• What user actions should my reviewer step through to review this PR?
  • review locally with:
    • app-root: https://local.zooniverse.org:3000/users/[insert your staging user login here]/stats
    • lib-user: https://local.zooniverse.org:8080/?users=[insert your staging user login here]/stats
• Which storybook stories should be reviewed? no new storybooks, but existing storybooks should continue to work
• Are there plans for follow up PR’s to further fix this bug or develop this feature? yes, Top Projects section PR (including some file reorganization and additional tests)

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

• Tests are passing locally and on Github
• Documentation is up to date and changelog has been updated if appropriate
• You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
• FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
• FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

• All pages of a FEM project load: Home Page, Classify Page, and About Pages
• Can submit a classification
• Can sign-in and sign-out
• The component is accessible
  • Can be used with a screen reader BBC guide to VoiceOver
  • Can be used from the keyboard WebAIM guide to keyboard testing
  • It is passing accessibility checks in the storybook

New Feature

• The PR creator has listed user actions to use when testing the new feature
• Unit tests are included for the new feature
• A storybook story has been created or updated
  • Example of SlideTutorial component with docgen comments, and its story

[coveralls]

coverage: 80.587% (-0.3%) from 80.918%
when pulling 998b225 on user-stats-bar-chart
into b26cf57 on master.

[shaunanoordin]

I'm now reviewing this PR, and hope to get it done before the end of the day 👌

[mcbouslog]

@shaunanoordin - I'm likely missing a lot of important context that could help your review, including but not limited to:

• related Figma designs
• project priorities and expectations (i.e. translations will be revisited separately, Sean will be doing a general user stats page design review), that the linked GitHub project FEM Stats Pages & Homepages might help provide some clarity regarding

Sorry this PR is a lot larger than I'd like a PR to be. Accordingly, I plan to revisit mobile and dark theme styling in a subsequent PR. Noting that I've started adding tests for UserStats, MainContent, and TopProjects in #5936 . Happy to provide some further context or clarification here or jump on a Slack call to share screens and walk through things.

[shaunanoordin]

PR Review (Quick Notes)

Thanks for the additional context, Mark!

• Noted that this PR is larger than you'd like it to be, but no worries. I'll keep the review focused on the big picture (overall functionality towards goals) instead of going into nitpicky details ("you forgot a semicolon!")
• Doubly noted that there are still other features that are still on the TODO (e.g. translations, that Export Stats button, etc), and those are for separate PRs. 👌
• Triply noted that Sean will be doing design review. 👌

At the moment, the overall functionality & code review look good, but I have encountered one medium-scale (crashing, but niche conditions) issue.

EDIT: erm, plus one "I'm-not-sure" issue I just discovered - as in, I'm not sure if it's a problem and what's the scale of the problem.

Dev Notes

💥 Medium-scale Issue 1: on the User Stats page, when a volunteer tries to switch to a timeframe where the project has no records/classifications, a crash occurs.

List of Notable Errors

Error on webpage:
NotFoundError: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

Errors in console:

Uncaught TypeError: Cannot read properties of undefined (reading 'period')
    at renderValue (DataChart.js:557:22)
    at eval (Detail.js:177:8)

Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

The above error occurred in the <Drop> component

Tested on app-root on 0663fa7 , on macOS + Chrome 122, on localhost (URL) with staging user darkeshard. Note: I had to be logged in as darkeshard to see my stats, otherwise the stats are blank. (EDIT: see Issue 2 below.)

Steps to replicate:

1. go to the user stats page of a test user. Test user (example: darkeshard) should have made a classification on any Project X more than a week ago, and no classifications within the last week.
2. set time period to a long time (example: 12 months) and then choose Project X (example: I Fancy Cats, which I last classified in Oct 2023)
   
3. change the time period to 7 days ago. Note that the projects list/dropdown has now gone blank.
   
4. at this point the crash will either occur after a few seconds of wait time, OR (RARELY) a second action (e.g. changing any dropdown value) is required to trigger the crash. (This inconsistency is bugging me.)
   

Video example: (same as above)

Screen.Recording.2024-02-26.at.23.40.23.mov

Analysis/Best Guesses:

• the list/dropdown of "time periods" is consistent, BUT the list/dropdown of projects is dynamic and dependent on the user + time period.
• As a result, changing the value of the "time periods" can/does also change the value of the projects list/dropdown, but the projects list/dropdown doesn't know how to handle items that were there but are no longer there, IF that item was the selected value.
• I think it's just the dropdown UI that's the problem, but I'll need to double check that the "fetch stats data" action isn't also borking due to invalid values.

Follow up questions (for Shaun, not Mark!):

• How well does the code handle a user with 0 projects? (e.g. freshly created user) (Assumption: should be OK, it should be the same if we set the time period to 7 days and the user hasn't classified anything in the past 7 days. API response would be the same)
• Is the "fetch stats data" action robust enough to handle nonsense input? How robust? Shaun: check the code.
• Does this problem occur whether "Classifications" and "Hours" tabs are selected? (Update: yes.)

Issue Status: major impact (crashes), but niche condition (it requires a bit to trigger). This isn't major enough to block this PR, but a fix will be necessary either for this PR or another.

💥 I'm-not-sure Issue 2: on app-root, the /users/USERNAME/stats page will always show the stats of the logged-in user (YOU), not the targeted USERNAME

Screenshot: I'm logged in as staging user darkeshard, and the stats show details for darkeshard, but the URL is for zootester1

Issue Status: I'm not sure if this is intended behaviour or not. ❓

• This also means if a user isn't logged in, the stats page will always be empty.
• Should a user's stats be only visible to the user, or should it be public? (I think the API makes it private to the user? Maybe? Gotta double check)
• But in any case, it just means that the whole /users/USERNAME/stats page on app-root needs a rethink as it doesn't actually reflect USERNAME's stats. Or maybe there are already plans for this, but I'm not aware of it - let me know and I'll downgrade this issue to a "known TODO item" instead!

Status

The review still ongoing, believe it or not!

I started writing this an hour ago as a "quick note" before I actually dug into the code, but I couldn't let go of the issues I encountered and I dug too deep and now aaaaaahhh.

• As mentioned, I won't let Issue 1 block this PR, but it will need to be fixed at some point.
• As for Issue 2, uh, I think I need some clarification if this is a known issue or not. As long as there's a plan, I'll be happy to not let this issue block this PR (because the PR is big enough as is without additional fixes, and I'm usually OK to allow WIP PRs to be merged on anything that's not yet deployed or public-facing as long.)

[shaunanoordin]

PR Review

Package: lib-user
Affects: user stats page (i.e. app-root's /users/USERNAME/stats path)

This PR adds major functionality to the user stats page, meaning a logged-in user can now see their stats on a bar chart, and filter the results based on projects and/or time range.

• 🆕 Features & functionality:
  • actual user stats are now being pulled from Panoptes, based on the criteria (selected project, time range) provided by the user
  • actual user stats are now being displayed on the bar chart
  • the stats page has functioning UI that allows the selection criteria (projects list, time range list) to be changed
• 🆕 Sub-component added: UserStat's MainContent
• 🆕 Helper functions/hooks added:
  • usePanoptesProjects(): given a list of project IDs, fetches a bunch of projects. Uses recursion to handle data paging.
  • getCompleteData(): if I understand correctly, fills out the "blank spaces" in a data array so it's formatted correctly for the bar chart. So if for example if we want the bar chart to show 7 days of data, but we only get [Sun, Wed, Fri, Sat] from the API, the helper will fill in the blanks so we'd get [Sun, Mon, Tue, Wed, Thu, Fri, Sat], so the bar chart has all 7 days in the x-axis.
  • getDateInterval()

❗ Notable issues & caveats: (see earlier PR Review (Quick Notes) for additional details)

• 💥 Issue 1: The app will crash if we select a project, then change to a time period when the selected project doesn't have any data.
• 💥 Issue 2: The app-root's /users/USERNAME/stats path will always show the data of the logged-in user, not the targeted USERNAME user.
  • (for this one, I'd appreciate some clarification so I know if it's a known issue, known TODO, or working as intended.)

As mentioned in the earlier PR Review (Quick Notes), I won't hold these issues as blockers for this PR, but they will need to be addressed at some point.

Testing

Tested on app-root (not lib-user) on localhost, with macOS + Chrome 122, while logged in as staging admin user darkeshard.

Test URL: https://local.zooniverse.org:3000/users/darkeshard/stats

Testing steps:

• View the user stats page for darkeshard as a non-logged in user. (No data is displayed)
• View the user stats page for darkeshard as logged-in user darkeshard. (Data is displayed.)
  • Use the dropdown to change the selected project.
  • Use the dropdown to change the selected time range.
  • Repeat multiple times. Try different combinations of default project + default time range; selected project + default time range ; default time project + selected time range ; selected project + selected time range ; etc. And then keep trying.
• View the user stats page for zootester1 (or any other user) as logged-in user darke shard. (Data for darkeshard is displayed, not zootester1)

Main functionality works as intended. Issues/caveats have been noted earlier.

Status

Sorry it took so long, but I wanted to say that this PR is LGTM. 👍

Mark, I'll leave it to you to decide how you want to handle Issue 1 and Issue 2. But, if you want my opinion, I'd merge this PR first so we have a baseline, and then address those issues on separate PRs. But if you do need me re-review this PR after putting in additional updates, please ping me on Slack and I'll get to it! 👌

[shaunanoordin]

(Related to Issue 2 from Quick Notes) I assume this is why the /users/USERNAME/stats page on app-root always shows details for the logged-in user, not the target USERNAME user.

[mcbouslog]

That is correct! There are plans in the short term to add a not authorized page, per a comparison of USERNAME vs user.login, then in the medium term to add admin access to any USERNAME (I think). The latter will require a more substantive refactor to separate USERNAME, user auth, and stats request, but that is the plan (eventually).

[shaunanoordin]

That's good, thank you for the clarification! 👍

[shaunanoordin]

[minor observation][rambling] I like the use getting a standardised ISO version of the date (YYYY-MM-DDTHH:mm:ss.sssZ ) and then extracting the date (without time, i.e. YYYY-MM-DD) via substring. Nice, clean!

The only caveat here is that it's technically possible for this to bork if the .toISOString() returns in the rare/obscure ±YYYYYY-MM-DDTHH:mm:ss.sssZ format which... is apparently a valid return value??

Not sure when/how any volunteer's machine would/could be configured to use the insane ±YYYYY or ±YYYYYY year format (which adds 2 or 3 additional characters to the string) which accounts for 29,187 A.D. and 500 B.C., but if we have to deal with time travellers then I think we have bigger problems to worry about than the stats page.

[mcbouslog]

Interesting edge case! I've made a note to revisit the conversion to account for the 6 digit year. I imagine a clever regex could solve the issue and plan to add it to a subsequent PR that refactors "All Time" per user.created_at.

[shaunanoordin]

[minor][code safety] The recursive accumulator design is brilliant, but I'm wondering if a safety valve is required to prevent infinite recursion (e.g. if (page > 100) { log_error() ; return null /* there shouldn't be this many pages! */ }), even if such an infinite recursion is extremely unlikely to happen.

[mcbouslog]

A safety vale is a great idea! It's a large page_size = 100 in the request as is. I can't imagine anyone would have more than a few hundred projects? Would a return if meta.projects.page_count > 5 make sense?

[shaunanoordin]

That page_size = 100 would make an extended recursion extremely unlikely to happen, nice. 👍

[minor][nerding out] But on a purely theoretical side, the safety valve for any recursive function or while loop has to check the dynamic counters (the values that change on each loop) for the safety to work. So checking for if (page > 5) return 'oh no!' or if (projects.meta.page > 5) return 'oh dear!' are both fine. Conversely, if we set if (projects.meta.page_count > 5) return 'curses, alas!', that safety valve would trigger even on the first page/first API call, since the value of projects.meta.page_count will be consistent for any given user. Curses, alas!

[mcbouslog]

Thank you @shaunanoordin !!! Extremely helpful review.

Regarding the first issue noted that results in the crash. I also see the issue frequently, but frustrating also have some difficulty reliably recreating. I also think it's related to no stats in the bar chart (as you've noted). I think it might specifically be related to the little drop content that shows on hover over a bar (what Grommet calls the Detail component) and that component trying to render when there are no related stats to render. I've made some related refactors that I think help minimize the crashing (I can not reliably reproduce, though I think I was able to once?), but have not fixed the UX where the selected project Select is blank/empty. I think I'm going to continue testing to confirm the refactor mitigates the crashing and check with Sean about what should show if there are no stats for a project in a selected date range.

[shaunanoordin]

@mcbouslog Additional good news - as of 7cb46bf, I've been unable to replicate the "no project data in time period causes a crash" issue. 👍 I know you've said you "mostly couldn't reproduce the error either, but might have triggered it once", but as your second set of eyes on this, I can tell ya that I still can't find a problem despite everything I'm throwing at it.

For now, I think I'm happy to say that the Refactor BarChart for no data commit did its job!

[mcbouslog]

Thanks @shaunanoordin for the additional testing! That's really good to hear you are also not able to reproduce the crashing subsequent the to noted commit. There's still a related issue (the empty select), but for now I think I'll merge this and address that aspect (as well as the other noted issues) in subsequent PRs. Thanks again!