-
Notifications
You must be signed in to change notification settings - Fork 0
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
create tags and captures_tags tables #16
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16 +/- ##
=========================================
Coverage ? 81.67%
=========================================
Files ? 22
Lines ? 131
Branches ? 0
=========================================
Hits ? 107
Misses ? 24
Partials ? 0 Continue to review full report at Codecov.
|
timestamps() | ||
end | ||
|
||
create unique_index(:tags, [:text]) |
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.
This index makes sure the database doesn't contain tags with the same name
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.
@SimonLab which issue requirement or acceptance criteria is this for?
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.
I've tried to follow the requirements from dwyl/app#245
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.
@SimonLab thanks for clarifying. In which case it could be worth having that issue linked in the PR description and having it "in-progress" on the Kanban board (which makes it easier for the reviewer) 😉
|> put_assoc(:tags, parse_tags(attrs)) | ||
end | ||
|
||
defp parse_tags(params) do |
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.
Tags are passed as a string separated by a comma
lib/app_api/captures/capture.ex
Outdated
|
||
defp insert_and_get_all(names) do | ||
maps = Enum.map(names, &%{text: &1}) | ||
Repo.insert_all(Tag, maps, on_conflict: :nothing) |
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.
A conflict can appears when inserting tags, for example on a race condition. If mutiple tag are inserted at the same time with the same name the postgres index will report an error. The on_conflict
catch this error and do nothing as at least one tag will be inserted with the correct name.
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.
@SimonLab ideally the index should be based on the person_id and tag_id so that different people can have the same tag. But I think we need a dedicated issue for tag ownership. 💭
ref: #14
tags
andcaptures_tags
table in Postgrescapture
changeset to link a capture to tags using put_assoc