Harry

[Scilly-guy]

Issue 76 - added context processor for contact details and edited templates to use values set in settings.
Issue 47 - added dangling slashes to urls as this seems to be considered less ambiguous
Created an entity relationship diagram using django-extensions, rendered it as an SVG and created an html file that allows panning and zooming.

[grundleborg]

Hi Harry, this looks great to me, congratulations on the first PR :)

First some suggestions on using git:

1. Ideally, create one branch per issue / bugfix / feature. Sometimes it makes sense to do a few together where they are related, but if not, best to do them in separate branches.
2. Give the branch a nice descriptive name, like remove-trailing-slashes or configurable-contact-details for example.
3. Give the PR a meaningful title e.g. Remove dangling slashes from URL patterns. The general approach in git is to try to keep this under 80 characters, although you can treat that as a guideline rather than a hard and fast rule. This will be the one line of text you see next to the commit in the abridged view of the git history, so it's important that it says what the PR does very clearly.
4. Give the PR a good description (actually you did this, so thank you). If it fixes a Github ticket, you can write a final line that say Fixes #47. This will automatically link the Github issue and when the PR is merged, it will auto-close the issue as well.

You'll find 3&4 to be good guidelines to follow for all commits, not just when submitting PRs.

Now moving on to the actual code. It basically all looks fine to me. I'd like to test it myself on my dev server before merging, which I haven't done yet, but based on what I'm seeing it all looks good. I've made a couple of suggestions inline in the code.

A final thing: make sure you run make format before you commit. It enforces some code-style rules. You can run it again now and do a fresh commit to this PR to fix the issues it flags up.

[grundleborg]

This is amazing, and really helpful. I'd like to suggest though that rather than committing this to the repo, can we have a command in the Makefile to generate it on the fly, and have it (and the HTML version in the next file) be in .gitignore so they don't get checked in to the repo. My reason for suggesting it this way is then we can all regenerate this whenever we want very easily, but won't clutter up the repo with commits changing the file / have potentially out-of-date versions in the repo, because this diagram is going to change quite often - whenever we make a model change).

Happy to help if you're not sure how to do it, but my suggestion would be:

1. Add the package that's needed to build this to the dependencies (the dev group specifically) with poetry.
2. Create a command in the Makefile called make generate-entity-relationship-diagram or similar, that runs the command to generate these files.
3. Add carshare-er.svg and entity_relationship_diagram.html to .gitignore so that they don't get committed.

What do you think?

[Scilly-guy]

I have done this, however it requires installing graphviz on your machine to convert the dot file produced by django-extensions.

[grundleborg]

Let's get rid of this one and just use the UK format phone number everywhere.

[Scilly-guy]

There is one place where a tel href is used, I believe this needs the number in this format to work.

[grundleborg]

Ahhh that makes sense.

[grundleborg]

Can you make these be read from the environment? e.g.
CONTACT_PHONE = os.environ.get("CONTACT_PHONE", "01111 222 333")

Then we can take actual IOSCV information out of the source code here and instead load it through our environment variable config instead.

[grundleborg]

This one doesn't look like it's used anywhere in the PR? If it's not needed, can we just call the other one CONTACT_EMAIL and do away with this one?