Apply the changes from the old pull request

[SamuelVch98]

I've applied all the changes requested in the old PR + I've added in the name of the DB in the config.json as requested during the previous meeting.

Overall, I've created a new blueprint for users, I've reviewed the way I create HTML files to avoid redundancy and I'm using SQLAlchemy in Flask to avoid duplicating sessions.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Samuel Van Campenhout seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[anthonygego]

We need to be able to review this.

• Split what is your work and what is imported code. Imported code need not to be reviewed (or, at least, need not a thorough look) as these actions are supposed to be done by the library maintainer. Isolate all the imports in one or several commits, and open a PR with only for this. This PR will then be mergeable very quickly.
• Only import what is needed. Here we just aim at redistributing the libraries artifacts needed for our app as a cache. There is therefore no need to redistribute what is not needed. You probably don't needall the bootstrap modules, nor the human-readable CSS and JS files. You can stick to the minified files. In sumary, there is no need to ship more than you import in this case.
• Put the review follow-up commits in the same branch as the reviewed PR. Otherwise changes are not tracked anymore, and review comments are harder to close as they need to read code side to side while keeping the original comment in memory.
• Do not push configuration files to the repository. The example file can be pre-populated with default fields if they are considered useful for the potential user (for instance organization_codes).
• Pre-review
  • Storing academic years in the configuration file seems not a good idea.
  • Storing organization codes in the configuration file is acceptable but you have to keep in mind the database would not be consistent anymore in the configuration file is changed and should therefore be cleaned from invalid entries before restarting the app.

Keep track of all these practice recommandations. They can be part of a chapter. Do not hesitate to complete and/or argue these ones with litterature on software maintenance. You can probably ask Kim for such pointers. That would be a nice addition.

[SamuelVch98]

• For the first point, I'll do it this way next time.
• I'm going to delete the bootstrap/js files not needed for the project and use the minified versions for the necessary files. I think I just need two files: bootstrap.min.js and bootstrap.min.css
• It's done, there aren't yet the changes you asked for in this pull request, but the changes from the previous PR have been made.
• I'm going to put the config.json file in the gitignore so that it won't be taken into account next time.
• Noted, will be done for the next PR

Thanks for the advice, it's helping me to improve.

[anthonygego]

There is unfortunately no next time for a pull-request to look good. The commits that will be merged should be split or cleaned to make the review convenient and not pollute the repository (commits history and therefore repo size as well).

The huge commit with external libraries should be split before going further. In an ideal world, fixes following the opening of a pull-request should be squashed in the corresponding implementation commit. In case of small pull-requests (one atomic feature or set of coherent fixes), Github allows us to directly squash all the commits when merging the PR.

Keeping a nice and clean commit history is really helpful when a git blame (or even a revert) is needed. This is easier to revert or identify regressions in atomic commits than trying to find the set of concerned commits, realizing there are not atomic and can't simply be revert, and then having to produce a specific patch for the introduced issue.

I'll close this PR#2 to focus on #1. However as already mentioned I suggest a dedicated PR for the external libraries.