tl-its-umich-edu / student_explorer

Tool used by Academic advisors to access student weekly performance
Apache License 2.0
10 stars 13 forks source link

Manage JavaScript dependencies using npm, bump some versions (#225) #226

Closed ssciolla closed 4 years ago

ssciolla commented 4 years ago

This PR makes progress toward completing issue #216 by integrating Node.js and npm with the build process to help with maintaining front-end JavaScript/CSS dependencies. A task list with the various actions taken is included below. The PR aims tor resolve issue #225.

Here is the status of the front-end dependency versions, with "Before" meaning what I believe we had before, "Now" meaning what this PR is proposing, and "Latest" being the most current version. Future PRs addressing issue #216 should likely focus on upgrading Bootstrap and D3.

Package Before Now Latest Notes
bootstrap 3.3.4 3.3.4 4.4.1 npm warns of moderate vulnerability; code changes are likely needed to upgrade
components-font-awesome 4.5.0 5.9.0 5.9.0 Now current
d3 3.4.13 3.5.17 5.15.0 code changes are likely needed to upgrade
jquery 2.2.3 3.4.1 3.4.1 Now current
nvd3 1.1.15-beta 1.1.5-beta2 1.8.6 We may need to replace this if we upgrade D3
tablesorter 2.31.1 2.31.2 2.31.2 Now current
jonespm commented 4 years ago

Codacy has 3 issues for this. I think they're all worthwhile but we could probably postpone doing the apt pinning for now.

mfldavidson commented 4 years ago

Not trying to be difficult here but why are we working on this now? We have other issues in the current sprint that haven't been started and this isn't (or at least, wasn't) in the current sprint.

ssciolla commented 4 years ago

Not trying to be difficult here but why are we working on this now?

@mfldavidson, point taken, there are other issues to be working on. I was kind of thinking about this yesterday and wanted to see what it would take, and then I was far enough along that I thought I would just try to get something working.

While it doesn't affect users particularly, I will say that this change is adjacent to some of the other issues out there in this project, specifically #45 and #212. We also talked about #216 a fair amount a couple weeks ago, and I thought that was something we wanted to make happen.

Also, I discovered once I got npm working that there were some significant security vulnerabilities, mostly it seems with the old jQuery. With these changes, we now only have one moderate security vulnerability with Bootstrap.

mfldavidson commented 4 years ago

I will say that this change is adjacent to some of the other issues out there in this project, specifically #45 and #212...With these changes, we now only have one moderate security vulnerability with Bootstrap.

Good justification. I'm sold.

zqian commented 4 years ago

Wow -83K lines of code, almost deleted the whole project! :)

credit for reducing code lines!

ssciolla commented 4 years ago

@zqian, thank you for pulling this down and testing! I'm merging now, but I think we should also do some significant testing and review on setest as well.