-
Notifications
You must be signed in to change notification settings - Fork 2
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 people migration #162
Conversation
- Create people table with peron_id and name - Copy existing person_id from items and tags - Add foreign key using references in items and tags
- Create Person schema - Update Item and Tag schema to use belong_to association
Create profile endpoint to manage the name
Add edit.html.heex form to edit the name of the loggedin person
Create a router plug to assign true/false to loggedin. This to avoid having duplicated code on the controllers
5f2b228
to
6797530
Compare
Create a router plug which redirect the person to her profile to add a name when it is not defined. This to make sure that people all have name and can be found later on when sharing items
Define update functions to update the person's name
Update test to avoid ecto foreign key error with new people table
Codecov Report
@@ Coverage Diff @@
## main #162 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 13 +2
Lines 195 219 +24
=========================================
+ Hits 195 219 +24
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This PR add a simple profile page. Because we are going to use the unique name of the logged in person, we are redirecting the user to the edit profile page if the name is not defined yet. Now that we have a way to find other user based on their profile name, we'll be able to start sharing items with other people |
a1673b8
to
2b38a81
Compare
@@ -16,7 +16,9 @@ | |||
<div class="container flex flex-wrap justify-between items-center mx-auto"> | |||
|
|||
<%= if @loggedin do %> | |||
<a href={Routes.profile_path(@conn, :edit, @person.id)}> |
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.
Add link to the profile picture in the nav bar to the profile edit page
end | ||
|
||
# insert new people rows by select unique person_id from items and tags | ||
execute( |
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.
Create person from the existing person_id from the items and tags table
Add more tests for profile schema and controller
2b38a81
to
bff37c1
Compare
import Ecto.Changeset | ||
alias App.{Item, Repo, Tag} | ||
alias __MODULE__ | ||
# https://hexdocs.pm/phoenix/Phoenix.Param.html |
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 find the docs for Param
pretty useless. Wish there was a standalone example. 💭
person | ||
|> cast(attrs, [:person_id, :name]) | ||
|> validate_required([:person_id, :name]) | ||
|> unique_constraint(:name, name: :people_name_index) |
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.
Can't say I understand why the :name
should be unique ... the :person_id
, sure, but 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.
I'm thinking of using the name later on in the UI to select people to add to groups. If the name is not unique we won't be able to distinguish between users. Maybe it should be called username
.
case Person.update_person(profile, person_params) do | ||
{:ok, _person} -> | ||
conn | ||
|> put_flash(:info, "Person updated successfully.") |
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.
Guessing this "Person updated successfully." is auto-generated? 🤖
Maybe we could change it to "Your profile was updated." or something a bit more friendly? 💭
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.
The flash messages are not displayed at the moment.
The function to display them might have been removed previously, I need to have a look at it but we can definitely update the message
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 looks good. 👌
Only one note on the uniqueness of person.name
💭
But fairly certain we can modify this later when needed.
my first and last name are not unique by any stretch ... 🙄
person.id
andname
person_id
fromitems
andtags
items
andtags