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

Use Ecto.Changeset for associations between items/person and statuses #94

Closed
SimonLab opened this issue Jul 11, 2022 · 5 comments
Closed
Assignees
Labels
awaiting-review An issue or pull request that needs to be reviewed priority-3 Third priority. Considered "Nice to Have". Not urgent. T1h Time Estimate 1 Hour T2h Time Estimate 2 Hours

Comments

@SimonLab
Copy link
Member

Once #93 is completed, I can review how the association are defined in the application and use the has_one, belongs_to, has_many changeset functions to create and manage these links with Ecto. I think this will enhance the readability of some code

@SimonLab SimonLab added the T1h Time Estimate 1 Hour label Jul 11, 2022
@SimonLab SimonLab self-assigned this Jul 11, 2022
@nelsonic nelsonic added the priority-3 Third priority. Considered "Nice to Have". Not urgent. label Jul 11, 2022
@SimonLab SimonLab added the in-progress An issue or pull request that is being worked on by the assigned person label Jul 11, 2022
SimonLab added a commit that referenced this issue Jul 11, 2022
- Update association to use Ecto.Changeset functions
- Make sure seeds file can creates statuses and person

related to: #94
@SimonLab
Copy link
Member Author

I've update the migrations files and schema to create the associations (see 422a854)
The seeds file is working and I can create a person associated with the verified status:
https://github.com/dwyl/app-mvp-phoenix/blob/422a854804d2264c10d67669f75e93145351f82d/priv/repo/seeds.exs#L20-L30

I'm now looking at updating the list_items functions to make sure the items returned don't have the status deleted. I need to read some doc to see how to filter item based on association

@SimonLab
Copy link
Member Author

      def list_items do                                                                                                                                                                      
         query =                                                                                                                                                                              
           from i in Item,                                                                                                                                                                    
             join: s in assoc(i, :status),                                                                                                                                                    
             where: s.text != :deleted,                                              
             preload: [status: s]                                                                                                                                                             
                                                                                                                                                                                              
         Repo.all(query)                                                                                                                                                                      
       end

Example from: http://blog.plataformatec.com.br/2015/08/working-with-ecto-associations-and-embeds/

SimonLab added a commit that referenced this issue Jul 11, 2022
Update the list_item function to checkt the
status ecto assocation value is not "deleted"

ref: #94 (comment)
SimonLab added a commit that referenced this issue Jul 11, 2022
@SimonLab SimonLab added awaiting-review An issue or pull request that needs to be reviewed T4h Time Estimate 4 Hours T2h Time Estimate 2 Hours T1h Time Estimate 1 Hour and removed in-progress An issue or pull request that is being worked on by the assigned person T1h Time Estimate 1 Hour T4h Time Estimate 4 Hours labels Jul 11, 2022
@nelsonic
Copy link
Member

@SimonLab apologies for not making this clear ...

One of the advantages of having statuses in their own separate module
[independently managed/maintained/tested dependency ...]
is that everything related to status can be in-memory on the Phoenix Server.
i.e. no database joins or lookups required, ever.
Not saying that having a preload is that inefficient.
But when we have less than 100 statuses (currently only 20)

image

and they are already defined in code,
we probably don't need to load them into the database.

I'm considering removing the status table from the database completely
and just linking to the statuses.json (now statuses.ex...) file to know what the statuses are.

If you look through the history of the "Update MVP" issue #89 and PR #90 commits - both still very much Work-in-Progress - you will see that I've "changed my mind" a few times in order to simplify this. Like I removed lists and therefore the need for list_items completely.
We can get both of those back in the actual App.
As discussed on our Standup call yesterday,
I've spotted 200+ things I want to improve in the MVP,
just while I've been rebuilding it using PETAL.

Adding associations can be useful if that's what we want. But we [probably] don't need them in the MVP.
The purpose of the MVP is to create the simplest possible useful version of the App.
And that means the bare minimum code to get everything working.

"La perfection est atteinte, non pas lorsqu'il n'y a plus rien à ajouter,
mais lorsqu'il n'y a plus rien à retirer.
"
~ Antoine de Saint-Exupéry

"Perfection is achieved, not when there is nothing more to add,
but when there is nothing left to take away
."
~ Antoine de Saint-Exupéry

If we are reading the README.md of the MVP
and we see something that does not absolutely need to be there,
we should remove it immediately. ✂️

I know you are trying to help. I very much appreciate it. ❤️
But the MVP update PR #90 is not ready for review yet.
And unless we can pair on it synchronously e.g. via Zoom or Tuple
It's taking up more of my time to address these issues than I'm spending on the build.
Please reserve these optimisations for after the PR is assigned for review.

If you only have T1h to do productive effective focus work per day, 👨‍💻
please spend it on a task that is already assigned to you and in your list 🙏
My time is incredibly short right now and I must must spend it on the things that I've assigned to myself.
I had less than an hour today and I got nothing done because I spent the whole hour answering questions (not just this one ...).

Again, associations have their place.
If you want to help make this easier, write it up in a generic way.
For example, I find the ElixirSchool chapter on Associations:
https://elixirschool.com/en/lessons/ecto/associations/
To be sorely lacking.
It adds a bunch of complexity to the code and doesn't actually explain WHY I would want to do that.
Ecto is a good ORM but I often find that it complicates things unnecessarily.
I wish it was closer to the actual SQL rather than trying to abstract it with a new DSL. 🤷‍♂️

Apologies for the long reply... ⏳
I just want to capture what I'm thinking so you don't have to
Please don't take this personally. I just need to focus on removing things from the MVP.
Adding associations can feel like a nice way to reason about the data. [ I agree ... ]
But if a change makes 200 code changes and adds zero explanation in the README.md,
it's actually a pretty major regression in terms of the objective of an MVP; learning.

Once again, the purpose of this repo,
is to write a comprehensive explanation
of the steps taken to build the MVP app.
Not to add code and not explain how it got there. 🤷‍♂️

Please don't do this again.

@nelsonic
Copy link
Member

Closing as PR is merged. ✅
Going to remove the status DB table completely from the project now as we don't need it. ✂️
Thanks for drawing my attention to it. 👍

Please let me finish my PR #90 🙏
which is mostly an update to the README.md 📖
and only incidentally adding some code. 👌

I will assign it for review when I feel that it's got nothing left to be removed. 🤞
Again, sorry that it's taking longer than I would have hoped.
I just don't have [anywhere near] as much focus time as I would like.

@nelsonic
Copy link
Member

on re-reading my comments above, it's clear to me that I was rude and ungrateful. 🤦‍♂️
that was not my intention. I'm very sorry.
the combination of tiredness, stress and lack of time making me a not nice person.
I had an idea for how to solve specific problems and explain the "naive" solution for the MVP.
But you're right, having associations is way better as the "right" way of doing it!
If you don't mind, please make this improvement after the PR #90 is merged. 🙏
Thanks again for being muuuuch better at this than me. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed priority-3 Third priority. Considered "Nice to Have". Not urgent. T1h Time Estimate 1 Hour T2h Time Estimate 2 Hours
Projects
None yet
Development

No branches or pull requests

2 participants