dwyl / mvp

📲 simplest version of the @dwyl app
https://mvp.fly.dev
GNU General Public License v2.0
84 stars 2 forks source link

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

Closed SimonLab closed 2 years ago

SimonLab commented 2 years ago

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 commented 2 years ago

I've update the migrations files and schema to create the associations (see https://github.com/dwyl/app-mvp-phoenix/commit/422a854804d2264c10d67669f75e93145351f82d) 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 commented 2 years ago
      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/

nelsonic commented 2 years ago

@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 https://github.com/dwyl/app-mvp-phoenix/issues/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 commented 2 years ago

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 commented 2 years ago

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. 🙌