Skip to content

#M05 Creating Hackathon Database Models#72

Merged
stefdworschak merged 32 commits intoCode-Institute-Community:masterfrom
Ri-Dearg:m05
Oct 17, 2020
Merged

#M05 Creating Hackathon Database Models#72
stefdworschak merged 32 commits intoCode-Institute-Community:masterfrom
Ri-Dearg:m05

Conversation

@Ri-Dearg
Copy link
Contributor

@Ri-Dearg Ri-Dearg commented Oct 16, 2020

These models are a draft that will fix issue #61
TLDR at the end.

Overview
The models.py file contains the models built from the schema here on the Development Wiki. I have followed the schema to the best of my ability. I definitely need other contributors to take a look at the fields for the Foreign Keys because I did change some of them.

I have given my reasoning for the choices in comments above each Foreign Key field. If anybody can see any errors or has a better idea of how to relate them, the advice is well appreciated! :-)

Testing
I have created each model in my own environment, it all seems grand. I deleted the DB and remigrated everything, I haven't had any errors yet. Models have a testing file that checks they can be created correctly and their str method.

To Do

  • Create a global method to automatically fill the created_by field on creation. Maybe find a way to prevent it from being edited.
  • Beautify the admin pages for the table relations and models. Some models depend on other models being present, so it can be really unintuitive to use the admin page to create them because you have to jump around.
    Displaying info from the related tables and making it editable on a different model's page would make this way easier! However, I think it could be started as a new issue. The models are needed to start work on other issues and it is a job in itself.

Possible Bugs

  • For the HackTeam model, it uses a ManyToMany field for "participants". I thought this was useful in case a User was part of a few Hackathons. However, it means that a user could be part of more than one team for the same hackathon. I think this could be solved with a custom save method.

I know it is a long read, so thank you again for any help!

TLDR:
Please take a look at the Foreign Key fields and their comments and let me know if you see any issues. I'll fix them up and make the commits. ;-)

Created hackathon app and moved tests inside its own module.
Updating branch with base files.
Connected the "hackathon" app by urls and settings"
Foregin Key fields will be revised as other models are created.
Added to admin. Minor adjustment made to Hackathon related name field
Updated comments. Must update all FK fields.
Foreign Keys match the schema closely but need review. See comments.
Syncing with changes from upstream repo
Sitched the *-Link fields for URLFields. Added comments.
Sitched the *-Link fields for URLFields. Added comments.

Added URLFields to project Model

Changed *_link to URLField, added comments. This commit fixes issue #61
…to m05

Updating commits to append request to issue.
@Ri-Dearg Ri-Dearg marked this pull request as draft October 16, 2020 14:55
Should not be commited here.
Copy link
Member

@stefdworschak stefdworschak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall a really good Pull Request. Some suggestions on how to proceed and let me know if you have any questions.

created_by = models.ForeignKey(User,
on_delete=models.CASCADE,
related_name="hackathon_created_by")
display_name = models.CharField(default="", max_length=254)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would allow blank and null instead of defaulting to an empty string. I think that's a little bit cleaner.

Copy link
Contributor Author

@Ri-Dearg Ri-Dearg Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this because the docs recommend to not do that for string-based fields here.
https://docs.djangoproject.com/en/3.1/ref/models/fields/#null

Let me know if you want me to swap it out anyway, and I'll do that. I've marked the other convos with this resolved for now tell me what you think here and I'll apply it to all of them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, just learnt something new. Did not know that, but let's follow the Django documentation then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I was doing the same thing myself until recently, then I found this. It only has a few rules, but it's useful:
https://github.com/rocioar/flake8-django

Copy link
Member

@stefdworschak stefdworschak Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really useful! Thanks for sharing.

Copy link
Contributor Author

@Ri-Dearg Ri-Dearg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the requested changes. Main points:

  • I'm using default="" because Django docs recommend it for string fields. If you prefer, I'll change it.
  • I removed the quotes around "Hackathon" but left them for "HackProject", take a look at the comment.
  • I think I made that last change for the model correctly, but let me know if it's right!
  • Tests are incoming!

I'm also wondering should I remove the comments over FK fields before a full pull?
Thank you for all the effort ye put into supporting the project, it's a great experience for me. :-)

created_by = models.ForeignKey(User,
on_delete=models.CASCADE,
related_name="hackathon_created_by")
display_name = models.CharField(default="", max_length=254)
Copy link
Contributor Author

@Ri-Dearg Ri-Dearg Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this because the docs recommend to not do that for string-based fields here.
https://docs.djangoproject.com/en/3.1/ref/models/fields/#null

Let me know if you want me to swap it out anyway, and I'll do that. I've marked the other convos with this resolved for now tell me what you think here and I'll apply it to all of them.

@Ri-Dearg Ri-Dearg marked this pull request as ready for review October 16, 2020 21:35
# Each model can only be created by one user: One To Many
created_by = models.ForeignKey(User,
on_delete=models.CASCADE,
related_name="hackathon_created_by")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have mentioned this in the other comment, but I would just rename all the related_names for created_by to be just the plural of the model name in lower case. I think that would be the best seeing as it they are basically a collection of that model created by a user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, makes sense, I'll do that now.

@stefdworschak
Copy link
Member

Also, don't forget to fix the merge conflict.

@Ri-Dearg
Copy link
Contributor Author

Ri-Dearg commented Oct 17, 2020

Okay, I changed the related names to plurals, I merged the conflict in the .gitignore adding both lines. And there are some basic str method tests in there too now.

I think it should be good to go. If you have any other corrections, let me know and I'll get right to it! :-)

Copy link
Member

@stefdworschak stefdworschak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Really good pull request! 👍

@stefdworschak stefdworschak added the hacktoberfest-accepted Accepted PR during Hacktoberfest label Oct 17, 2020
@stefdworschak stefdworschak merged commit a59f43f into Code-Institute-Community:master Oct 17, 2020
@Ri-Dearg Ri-Dearg deleted the m05 branch October 17, 2020 16:20
@TravelTimN TravelTimN linked an issue Oct 17, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted Accepted PR during Hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

#M05 Creating Hackathon Database Models

2 participants

Comments