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

Create people migration #162

Merged
merged 9 commits into from
Oct 5, 2022
Merged

Create people migration #162

merged 9 commits into from
Oct 5, 2022

Conversation

SimonLab
Copy link
Member

@SimonLab SimonLab commented Oct 4, 2022

  • Create people table with person.id and name
  • Copy existing person_id from items and tags
  • Add foreign key using references in items and tags

- 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
@SimonLab SimonLab added the in-progress An issue or pull request that is being worked on by the assigned person label Oct 4, 2022
@SimonLab SimonLab self-assigned this Oct 4, 2022
@SimonLab SimonLab temporarily deployed to dwylauth October 4, 2022 20:43 Inactive
- Create Person schema
- Update Item and Tag schema to use belong_to association
@SimonLab SimonLab temporarily deployed to dwylauth October 4, 2022 21:20 Inactive
Create profile endpoint to manage the name
@SimonLab SimonLab temporarily deployed to dwylauth October 4, 2022 21:52 Inactive
Add edit.html.heex form to edit the name of the loggedin person
@SimonLab SimonLab temporarily deployed to dwylauth October 4, 2022 22:17 Inactive
@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 09:41 Inactive
Create a router plug to assign true/false to loggedin.
This to avoid having duplicated code on the controllers
@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 09:42 Inactive
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
@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 10:09 Inactive
Define update functions to update the person's name
@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 10:26 Inactive
Update test to avoid ecto foreign key error with new people table
@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 12:04 Inactive
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #162 (bff37c1) into main (40e9ea5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #162   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        13    +2     
  Lines          195       219   +24     
=========================================
+ Hits           195       219   +24     
Impacted Files Coverage Δ
lib/app/item.ex 100.00% <ø> (ø)
lib/app/tag.ex 100.00% <ø> (ø)
lib/app_web/controllers/init_controller.ex 100.00% <ø> (ø)
lib/app_web/controllers/tag_controller.ex 100.00% <ø> (ø)
lib/app/person.ex 100.00% <100.00%> (ø)
lib/app_web/controllers/profile_controller.ex 100.00% <100.00%> (ø)
lib/app_web/router.ex 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 13:12 Inactive
@SimonLab
Copy link
Member Author

SimonLab commented Oct 5, 2022

This PR add a simple profile page.
I have created a new migration to create the people table and the person schema linked to this table.
To keep the existing data, I run an sql command which create new person based on the existing person_id from items and tags.

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

@SimonLab SimonLab requested a review from nelsonic October 5, 2022 13:16
@SimonLab SimonLab added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Oct 5, 2022
@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 13:19 Inactive
@@ -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)}>
Copy link
Member Author

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(
Copy link
Member Author

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
@SimonLab SimonLab temporarily deployed to dwylauth October 5, 2022 13:21 Inactive
@SimonLab SimonLab marked this pull request as ready for review October 5, 2022 13:24
@nelsonic nelsonic assigned nelsonic and unassigned SimonLab Oct 5, 2022
@nelsonic nelsonic added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Oct 5, 2022
import Ecto.Changeset
alias App.{Item, Repo, Tag}
alias __MODULE__
# https://hexdocs.pm/phoenix/Phoenix.Param.html
Copy link
Member

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)
Copy link
Member

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 ...? 🤷‍♂️

Copy link
Member Author

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.")
Copy link
Member

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? 💭

Copy link
Member Author

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

Copy link
Member

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

image

@nelsonic nelsonic merged commit 7d05242 into main Oct 5, 2022
@nelsonic nelsonic deleted the person-table-schema-#290 branch October 5, 2022 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review Issue or pull request that is currently being reviewed by the assigned person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants