Clean up Open PRs

[ryanfchase]

Overview

We have a long list of PRs that likely aren't being used anymore. We'll use this ticket to list, review, and handle each of those PRs.

Action Items: Please review these lists

PRs by Author

• Review All Dependabot PRs #1641
• A few README PRs opened by Edwin (I've approved the most complete one, but I saw there were merge conflicts)

PRs which affect archived directories (e.g. server)

• Update to Plotly Graphs (Fix titles, eliminate confusing mode bar, adds color to request by source graph) #1496
• updating tf version and updating code to comply #1479
• Adding 2022 SA + KL, issue #1165 #1168
• Remove duplicate color for waterwaste #1226
• 1235-Dashboard-add radio buttons w moving average breakdown by wk and mo, normalize by population per 10k people and update labels #1242

Generally Outdated

• Changed initial date range for loading screen from one week proir to … #1431
• Collapse legend #1159

[Skydodle]

Regarding dependabot PRs:

I’ve had small experience with dependabots on my personal projects. It basically scans the repository periodically to see if there are dependencies that can be updated without breaking the app, which help with closing security vulnerabilities from outdated packages.
It’s generally safe to trust the ones that has a check mark (which means it built with the updated dependency and passed the build test without conflict).

The only ones we would have to investigate manually are the ones with x marks that failed the build tests. But dependabots usually provide pretty good logs for one to investigate and fix. The failed reason varies. The one I experienced was an npm package that was super outdated and no longer supported/maintained.

[Skydodle]

But also my experience is based on personal projects, which are relatively small compare to real world projects. I've read some devs pointed out that as the app grow larger the dependencybot conflicts would also increase since the number of dependencies would also most likely be growing. And also it would be catching stuff that we forgot to write tests for.

Another suggestion is to just ignore them and keep all dependencies on current version so we can focus on other things. I would even delete those current dependabot PRs (since those PRs suggested dep versions maybe outdated later on). Then when we are ready to launch or scale, run the dependencybot and spend time dealing with them all at once. Not sure if this is for the best though.

[ryanfchase]

@Skydodle thanks.

There are 36 dependabot open PRs, and maybe 2/3s have green checkmarks. In lieu of having a test framework, it may be prudent to go through each one and test that it doesn't break the map. It will take longer, but we don't have a better way of confirming that it was a non-breaking change.

Alternatively, one could go and, on their own branch, implement the code change from each of the green checkmark dependabot PRs. This person would then test them all at once, and see if the map works as expected. If not, we'd have to go through and weed out the bad change.

Thoughts?

edit: I could be convinced that this is too much work, if green-checkmarks are pretty trustworthy. We could then use the above strategy for the questionable dependabot PRs.

[Skydodle]

I could dig in to see if Github has any guidelines on testing all the dependabot changes on a target branch before just trusting them and merge to main. I can work on that.

[ryanfchase]

Thank you @Skydodle - I've moved the ticket to Prioritized Backlog. Please assign yourself when you begin working, partial investigative info is still useful. Feel free to do as much as you can and report your findings and unassign.

[Skydodle]

Thanks @ryanfchase I will work on it in the next few days and post findings.

[Skydodle]

@ryanfchase can you/ someone open a sub- issue specifically for dealing with the current dependabots PRs?

I have some findings for short-term and long-term solution, and I want to post them under this new sub-issue for discussion. I don't want to clog up this current issue's board, as clearing the dependabot PRs is just one of many sub-stasks of this issue.

Let me know what you think.

[ryanfchase]

@Skydodle I've opened a new issue here: #1641

We'll move the issue to Prioritized Backlog once you're satisfied with the strategy there. Please unassign yourself from this ticket and move it back to Prioritized Backlog (since you're soon to contribute to 1641). Also, feel free to open new issues on your own via the Blank Issue template. Assign to myself / @edwinjue for us to approve the issue.

[Skydodle]

@edwinjue @ryanfchase I reviewed the remaning PRs listed in this issue (except the README one), all of them were requested to merge with the dev branch, and therefore has no effect on the current working branch main whether we merge them or not. Also these were very old change requests (late 2021 to early 2023) pertaining to the previous repo before we switch to front-end only.

My suggestion is to close them without merge since they have no effect on the current working repo. Please kindly advise if I'm good to proceed.

List of the proposed PRs to be closed without merge

[ryanfchase]

Thank you @Skydodle -- we're good to close the PRs without merge! Thank you for reviewing them.

[Skydodle]

@edwinjue @ryanfchase

This is entire issue is ready to be closed, please kindly review the details below:

• I've closed out the outdated PRs listed in my previous comment
• Moved on to review the 3 README PRs & took action as below:
  • Update README.md #1548 : It correctly updated the clone directory to cd into --> Reviewed, Approved with comment & Merged.
  • Update README.md #1563 : It correctly updated the demo site link --> Reviewed, Approved with comment & Merged.
  • Update README.md #1552 : This PR has the exact same code change as Update README.md #1548 above, which caused the merge conflict due to redudancy --> Closed without merge due to same issue already solved by Update README.md #1548

Side-note:

• The README overall still has some outdated information and needs to be consider for future tickets.

Let me know if I'm good to proceed and close this issue. Thank you =)

[ryanfchase]

Looking great @Skydodle -- this issue is good to close (moved to In Review while I was reviewing. Closing will move to Done)

[Skydodle]

@ryanfchase Thank you clarifying the review & closing workflow!