Analytics Modal + CSV Bug Fix#603
Conversation
|
[diff-counting] Significant lines: 441. |
benkoppe
left a comment
There was a problem hiding this comment.
Good work with these changes, but I think a bit more is needed to ensure proper usability and polish. See my comments about the modal options especially. Also be careful about things like dependencies in the package.json! I'm finding myself making mistakes like that more and more as I let AI automate commands. I hope these comments are overall useful!
| "description": "", | ||
| "main": "index.js", | ||
| "dependencies": { | ||
| "@emotion/react": "^11.14.0", |
There was a problem hiding this comment.
I don't think all of these dependencies are necessary, and all of them should be contained to the package.json of the frontend. For example, react is only needed by the frontend (and is probably already a dependency there, too).
| <div className={styles.form}> | ||
| <LocalizationProvider dateAdapter={AdapterDayjs}> | ||
| <div style={{ marginBottom: '1rem', marginTop: '1.5rem' }}> | ||
| <DatePicker |
There was a problem hiding this comment.
You might want to rethink how you handle the modal here. Using it on the website, I was frustrated that the date range I had set for the main analytics view didn't carry over to the default values on this modal. Maybe it could be a prop for the export button?
… CSV, added documentation to stats backend]
0d64e90 to
5668b7a
Compare
Error in AnalyticsTable: Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render. Error in stats: trying to create an existing stat
mjaydenkim
left a comment
There was a problem hiding this comment.
unfortunately problems w/ the analytics backend are kinda prohibitive to testing this pr, lmk if that's something you wanna work on/if not id love to help like i said!!!
There was a problem hiding this comment.
should these really be pushed?
| driverData.push(driverRow); | ||
| } | ||
| }); | ||
| sortedData.forEach((d) => { |
There was a problem hiding this comment.
i put in some changes to fix a warning about the maximum update depth being exceeded
| }); | ||
|
|
||
| /** | ||
| * GET |
There was a problem hiding this comment.
currently this is highly inefficient b/c it requests day by day -- requests over the medium to long term take a very long time to process
| * | ||
| * response: | ||
| * - JSON array of computed or existing stats | ||
| */ |
There was a problem hiding this comment.
right now when i try to request data from the backend i get the following error -- Populate failed for item { id: _____ } error: TypeError: Cannot read properties of undefined (reading 'getInternalProperties')
...
Error: Cannot set headers after they are sent to the client
There was a problem hiding this comment.
style and stuff looks pretty good, and i know that the backend isn't your priority in this PR but these are problems that prevent me from using the page at all -- if you wanna fix them that'd be amazing ! but if you don't have the capacity/don't want to please feel free to hit me up b/c i already tried bugfixing this for a minute and i'd love to help out
There was a problem hiding this comment.
@mjaydenkim Thanks for the feedback!! The error you get is something everyone seems to be facing when requesting data. We talked about it at the end of work session last week and it seems like it's a problem with AWS/DynamoDB. I was getting it a lot when I was working on creating custom rides and Ryan also mentioned something about it. Definitely something we need to look into but I'm not sure its specific to this PR
as for the rides logic problems, if you have time and want to help that would be great! there is a branch called stats-backend that I was using which has a lot of console logs to debug if you want to check that out.
one problem I was considering is updates and how to ensure they persist---if the stats table is based on information from the rides table, then the edit button for this page is essentially useless because what's recorded will be erased when the page is refreshed and the rides data (which will not be updated) is requested again.
i think the easiest solution to this would be to get rid of the update button and rely on drivers/admin to ensure rides have the correct statuses. because it might be infeasible for drivers to constantly press update, one idea I had was to have all rides automatically transition to completed after their end time occurs. because no shows are likely rare, having the admin go to the home page and update the individual ride (instead of just the stats table) should not be too much of a hassle.
There was a problem hiding this comment.
that makes a lot of sense, i figured it was a broader issue -- i'll look into migrating backends cuz this is really annoying LOL. tysm for letting me know ! i was just apprehensive to sign off on the changes when they're currently difficult to test yk? also that's amazing i'll go into that branch and take a look
There was a problem hiding this comment.
also i think you're right about the update button -- i don't imagine that admin would be trying consistently to edit rides from the analytics page too. i think transitioning to completed makes sense to me as a default end state too, that sounds good to me
mjaydenkim
left a comment
There was a problem hiding this comment.
did a second pass on the code line by line instead of focusing on testing the broken dynamodb stuff and it looks really good to me! the ui element modifications are great and so is the documentation !
There was a problem hiding this comment.
exporting doesn't work for me b/c of the aforementioned errors but this code looks good to me !
| }); | ||
|
|
||
| /** | ||
| * PUT |
Summary
This pull request is the first step towards implementing the analytics page
Testing
I did some preliminary manual tests and everything seemed to work. Using MUI DatePicker also helped with streamlining the date validation process. However, more in depth testing (by creating lots of rides with different characteristics such as if they are cancelled, no shows, at night, etc) will likely be needed once I fix backend querying (see below).
Notes
This is the first part of fixing the analytics page. Although CSV exporting now works, the analytics page does not query previous ride information (all the data currently is dummy data). My next step will be fixing the backend so that querying works correctly.