-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add contact model #2970
Add contact model #2970
Conversation
Code Climate has analyzed commit 486c6a7 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 80.3% (50% is the threshold). This pull request will bring the total coverage in the repository to 83.3%. View more on Code Climate. |
@JoeyStk |
@MizukiTemma I was undecided about this yesterday. How about we merge this without the list and once #2951 is merged, I will open a new PR for the list? :) |
Ok 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review yet, but I would suggest to also include permissions for the model here. Otherwise we would have to introduce another migration
a46cc95
to
c7c35af
Compare
In many commits I was able to fulfill all of your suggestions. Please feel free to review again :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this looks good now.
One thing to note is that we will almost certainly have to change the model again soon, to implement the remaining contact related issues (for example #2959), which feels a bit weird to me, but I don't think it makes sense to delay this further either :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks almost good 💪 I have only one concern to the permission settings in context of hiding this feature to most of users until completed.
b1b3108
to
486c6a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much 🎉
Short description
This PR adds the initial db model for our new contact section.
Proposed changes
Side effects
Resolved issues
Fixes: Parts of #2952
Pull Request Review Guidelines