Skip to content
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

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Add contact model #2970

merged 1 commit into from
Aug 9, 2024

Conversation

JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Aug 6, 2024

Short description

This PR adds the initial db model for our new contact section.

Proposed changes

  • Add blueprint for model
    • fields as described in issue description
    • default fields such as created, last updated and others
  • Add migration file for it
  • Add dummy contact to test data

Side effects

  • I decided for region as we want every contact to be (implictly) assigned to a region
  • The relation POI to contact is 1 to 1 (as requested in the requirements)
  • The relation contact to region is 1 to 1 (as requested in the requirements)
  • Once this is merged, it unblocks a couple of other issues related to this feature

Resolved issues

Fixes: Parts of #2952


Pull Request Review Guidelines

Copy link

codeclimate bot commented Aug 6, 2024

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.

@MizukiTemma
Copy link
Member

@JoeyStk
Is the list comming together in this PR or will it be implemented separately?

@MizukiTemma MizukiTemma added the prio: high Needs to be resolved ASAP. label Aug 7, 2024
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Aug 7, 2024

@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? :)

@MizukiTemma
Copy link
Member

@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 😉

Copy link
Member

@david-venhoff david-venhoff left a 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

@JoeyStk
Copy link
Contributor Author

JoeyStk commented Aug 7, 2024

In many commits I was able to fulfill all of your suggestions. Please feel free to review again :)

Copy link
Member

@david-venhoff david-venhoff left a 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 :/

integreat_cms/cms/views/regions/region_actions.py Outdated Show resolved Hide resolved
Copy link
Member

@MizukiTemma MizukiTemma left a 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.

integreat_cms/cms/constants/roles.py Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the feature/add-contact-model branch from b1b3108 to 486c6a7 Compare August 9, 2024 11:46
@JoeyStk JoeyStk requested a review from MizukiTemma August 9, 2024 11:57
Copy link
Member

@MizukiTemma MizukiTemma left a 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 🎉

@JoeyStk JoeyStk merged commit 149a846 into develop Aug 9, 2024
5 checks passed
@JoeyStk JoeyStk deleted the feature/add-contact-model branch August 9, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio: high Needs to be resolved ASAP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants