dwyl / mvp

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

EPIC: Update MVP App to Latest `Phoenix` with `auth_plug` #89

Closed nelsonic closed 2 years ago

nelsonic commented 2 years ago

This project is a bit out-of-date. Let's fix that and add @dependabot to keep it up-to-date!

Todo

nelsonic commented 2 years ago

ERD of existing MVP on main branch: [click the image to enlarge]

image

This ERD covers the "Single Player Mode" described in the /dwyl/product-roadmap#1-single-player i.e. there is no concept of sharing, permissions or collaboration.

Do we want to add the complexity of "Multi-Player" to the MVP? I know that personally I want to be able to share an item with my co-worker/family-member/friend etc. But the question is: should we be thinking about that in MVP or only in a subsequent iteration? 💭

nelsonic commented 2 years ago

I think I answered my own question in the Product Roadmap link above. 💭 Specifically, the App must be immediately useful to the individual person. 👩‍💻 So that's what I'm (we're) going to focus on first. 🏁

And then we can add the concept of groups of (one ore more) people later. 👪

nelsonic commented 2 years ago

For reference, at the time of writing, visiting: https://dwylapp.herokuapp.com I see: image

When I authenticate with my Google Account, I see: https://dwylapp.herokuapp.com/people/info image

And when I click on "capture" I see: https://dwylapp.herokuapp.com/capture/new

image

When I submit I'm shown: https://dwylapp.herokuapp.com/categorise

image

When visiting the list: https://dwylapp.herokuapp.com/items image

This item created by a random person: https://dwylapp.herokuapp.com/items/233 image

Summarises it well. 💭

nelsonic commented 2 years ago

Quick walkthrough of the "User Journey" through using the current version of the App, with reference to the ERD:

mvp-erd

This version does not allow people to create items without being authenticated. So that's the first step as noted in the screenshots above.

Once authenticated the person creates an item. The item can be edited/updated. That's it.

At present the schema is there for lists and list_items but also the schema is there for timers but the functionality/UI to start|stop a timer is not https://github.com/dwyl/app-mvp-phoenix/issues/7 tags and status tables are also present but there is no UI to manage them.

So the MVP as it stands is very incomplete. Let's fix that!

nelsonic commented 2 years ago

Dependencies we want to bring with us:

  # Easily Encrypt Senstive Data: github.com/dwyl/fields
  {:fields, "~> 2.7.1"},
  # Auth with ONE Environment Variable: github.com/dwyl/auth_plug
  {:auth_plug, "~> 1.2.3"},
  # Useful functions: https://github.com/dwyl/useful
  {:useful, "~> 0.1.0"},

  # create docs on localhost by running "mix docs"
  {:ex_doc, "~> 0.22.6", only: :dev, runtime: false},
  # track test coverage
  {:excoveralls, "~> 0.13.2", only: [:test, :dev]},
  # git pre-commit hook runs tests before allowing commits
  {:pre_commit, "~> 0.3.4"},
  {:credo, "~> 1.1.0", only: [:dev, :test], runtime: false},

https://github.com/dwyl/app-mvp-phoenix/blob/745c7e87b42f07888edfe7f4527998b827e6a6e3/mix.exs#L54-L67

Note: all of these are way out-of-date, this is just for reference.

nelsonic commented 2 years ago

Also want to keep these settings

test_coverage: [tool: ExCoveralls],
preferred_cli_env: [
  c: :test,
  coveralls: :test,
  "coveralls.json": :test,
  "coveralls.html": :test,
  t: :test
]

and aliases:

c: ["coveralls.html"],
s: ["phx.server"],
t: ["test"],
nelsonic commented 2 years ago

After re-creating the schemas using mix phx.gen.html and seeing how much boilerplate is created: 🙄 https://github.com/dwyl/app-mvp-phoenix/commit/c649808721bfbd897b4541d41fc4b765dfc710a3#

65 changed files with 2,618 additions ...

I decided to delete it all and try a more lightweight approach. 💭

nelsonic commented 2 years ago

The problem with using mix phx.gen.schema is that no tests are created so we end up with:

Finished in 0.1 seconds (0.07s async, 0.07s sync)
3 tests, 0 failures

Randomized with seed 935707
----------------
COV    FILE                                        LINES RELEVANT   MISSED
  0.0% lib/app/item.ex                                19        2        2
  0.0% lib/app/list.ex                                20        2        2
  0.0% lib/app/person.ex                              23        2        2
  0.0% lib/app/status.ex                              17        2        2
  0.0% lib/app/tag.ex                                 17        2        2
  0.0% lib/app/timer.ex                               20        2        2
100.0% lib/app_web/controllers/page_controller.        7        1        0
100.0% lib/app_web/router.ex                          21        2        0
100.0% lib/app_web/views/error_view.ex                16        1        0
  0.0% lib/app_web/views/layout_view.ex                7        0        0
  0.0% lib/app_web/views/page_view.ex                  3        0        0
[TOTAL]  25.0%
----------------
Generating report...
nelsonic commented 2 years ago
13 files changed, 219 insertions(+)

way more sane! 💭

nelsonic commented 2 years ago

GitHub CI will fail because the coverage threshold defined in coveralls.json is 100% 💯 and with these new schemas the coverage drops to 25% 😮

So need to write some tests before this will pass. 👨‍💻

nelsonic commented 2 years ago

Confirmed. Tests pass but coverage dropped to 25: https://github.com/dwyl/app-mvp-phoenix/runs/7088823860

mvp-coveage-25%

Might take a while to get this back up to 100% ... ⏳ But, as you know, I think it's worth the effort. 💭

SimonLab commented 2 years ago

I'm going to look today at adding tests to boost the coverage. confirm that running mix c returns the following report: image

@nelsonic if I remember correctly you started to add liveView page to the branch, when you have a bit of time could you push your latest changes as I don't want to create any conflicts with your code, thanks

nelsonic commented 2 years ago

@SimonLab I've pushed all my changes to the branch. ⬆️ It's still very much "work-in-progress" ⏳ and will actually be confusing and counter-productive for you to read it at this point. 🙄

I'm both trying to simplify the MVP, update the UI/UX and add Auth ... 🔒 + 😍 But my overarching objective is to make it easy for a beginner/new-joiner to understand what's going on. 🤞 Apologies for taking a bit longer on this than I would have hoped. ⏲️

nelsonic commented 2 years ago

Removing lists for now. Will re-add them later on in the log.

mix phx.gen.schema List lists title:string person_id:references:people status:references:status tag:references:tags

Just that they aren't explained anywhere and it's going to be less confusing to people if we introduce the topic later!

nelsonic commented 2 years ago

2.1 Create list_items

mix ecto.gen.migration create_list_items_association
nelsonic commented 2 years ago

3 A "list" is a way of grouping items of content. An "essay" or "blog post" is a list of notes. A "task list" (or "todo list" if you prefer) is a list of tasks.

We are well aware that the word "list" is meaningful in many programming languages.

We have chosen to use "list" as it's the most obvious word in english.
We did not find a suitable synonym: https://www.thesaurus.com/browse/list 🔍 🤷‍

4 We cannot use the word "type" as a field name, because it will be confusing in programming languages where type is either a reserved word or a language construct. see: https://en.wikipedia.org/wiki/Type_system Instead we are using the term tag which is relatively familiar to people and has the benefit of being both a noun and a verb. So the expression "tagging" the data works.

nelsonic commented 2 years ago

Simplified ERD:

image
nelsonic commented 2 years ago

On re-re-re-re-re-re-re-re-reading ... 📖 👀 It doesn't make sense to introduce tags before the UI/UX is there either. 💭 so removing them temporarily ... ✂️

Re-create:

mix phx.gen.schema Tag tags text:string

tag

tag - tags2 can be applied to an item to Categorise it.

nelsonic commented 2 years ago

2 We expect (and will encourage) people to define their own tags. When creating tags, the UI will auto-suggest existing (public/curated) tags to avoid duplication and noise. For now we only need "task".

nelsonic commented 2 years ago

Final simplification? 💭

image
nelsonic commented 2 years ago

Removed the schema migrations from the ERD to declutter:

image

Feel like this is the final simplification ... I mean we could start with just items ... But we have that in the https://github.com/dwyl/phoenix-liveview-todo-list-tutorial already. This is meant to take it to the next level.

nelsonic commented 2 years ago
### 2.1 Create `item_tags` Association

An `item` should be able 
to have one or more tags associated with it.
To enable this we need a new table 
to store the data.

#### `item_tags`

`item_tags` - are the `tags` applied to an `item`. 

+ `item_id` (**FK** item.id)
+ `tag_id` (**FK** tag.id)
+ `inserted_at`
nelsonic commented 2 years ago

Removed this chunk:

We now need to add person_id to tags and status to ensure that a human has ownership over those records.

mix ecto.gen.migration add_person_id_to_tag
mix ecto.gen.migration add_person_id_to_status

Code additions:

ER Diagram With the person_id field added to the tags and status tables:

app-er-diagram-person_id-status-kind

Associate Items with a List

An item will always be on a list even if the list only has one item. By default the list an item will be associated with is "uncategorised".

Let's create the migration to link items to lists:

mix ecto.gen.migration create_list_items_association

With the migration file we need to edit the following files:

Open the lib/app/ctx/item.ex file, locate the schema block:

schema "items" do
  field :text, :string
  field :human_id, :id
  field :status, :id

  timestamps()
end

Add the line belongs_to :list, App.Ctx.List such that your schema now looks like this:

schema "items" do
  field :text, :string
  field :human_id, :id
  field :status, :id
  belongs_to :list, App.Ctx.List # an item can be linked to a list

  timestamps()
end

Open the lib/app/ctx/list.ex file and locate the schema block. Add the line has_many :items, App.Ctx.Item such that your schema now looks like this:

  schema "lists" do
    field :title, :string
    field :person_id, :id
    field :status, :id
+   has_many :items, App.Ctx.Item # lists have one or more items

    timestamps()
  end

Open the newly created migration file: priv/repo/migrations/{timestamp}_create_list_items_association.exs
and add the following code to the change block:

def change do
  create table(:list_items) do
    add :item_id, references(:items)
    add :list_id, references(:lists)

    timestamps()
  end

  create unique_index(:list_items, [:item_id, :list_id])
end

That will create a lookup table to associate items to a list.
Code snapshot: https://github.com/nelsonic/time-mvp-phoenix/commit/935eac1251580c13b45d9341f0597e4118f1a66f

Note: we are not imposing a restriction (at the database level) on how many lists an item can belong to in the list_items table. The only restriction is in the items schema has_one :list, App.Ctx.List. But this can easily be updated to has_many if/when the use case is validated. See: time-mvp-phoenix/issues/12

After saving the above files, run mix ecto.migrate. Now when you view the Entity Relationship Diagram it should look like this:

app-er-diagram-with-list_items

nelsonic commented 2 years ago

https://hexdocs.pm/ecto_erd/Mix.Tasks.Ecto.Gen.Erd.html (thanks @SimonLab!)

nelsonic commented 2 years ago

Lame:

↳ Enum."-each/2-lists^foreach/1-0-"/2, at: lib/enum.ex:937
** (FunctionClauseError) no function clause matching in String.replace/4

    The following arguments were given to String.replace/4:

        # 1
        nil

        # 2
        "'"

        # 3
        ""

        # 4
        []

    Attempted function clauses (showing 1 out of 1):

        def replace(subject, pattern, replacement, options) when is_binary(subject) and is_binary(replacement) or is_function(replacement, 1) and is_list(options)

    (elixir 1.13.4) lib/string.ex:1477: String.replace/4
    (fields 2.9.0) lib/aes.ex:91: Fields.AES.fetch_keys/0
    (fields 2.9.0) lib/aes.ex:69: Fields.AES.get_key_id/0
    (fields 2.9.0) lib/aes.ex:34: Fields.AES.encrypt/1
    (fields 2.9.0) lib/encrypted.ex:21: Fields.Encrypted.dump/1
    (ecto 3.8.4) lib/ecto/type.ex:938: Ecto.Type.process_dumpers/3
    (ecto 3.8.4) lib/ecto/repo/schema.ex:996: Ecto.Repo.Schema.dump_field!/6
    (ecto 3.8.4) lib/ecto/repo/schema.ex:1009: anonymous fn/6 in Ecto.Repo.Schema.dump_fields!/5
    (stdlib 3.17.1) maps.erl:410: :maps.fold_1/3
    (ecto 3.8.4) lib/ecto/repo/schema.ex:1007: Ecto.Repo.Schema.dump_fields!/5
    (ecto 3.8.4) lib/ecto/repo/schema.ex:940: Ecto.Repo.Schema.dump_changes!/6
    (ecto 3.8.4) lib/ecto/repo/schema.ex:360: anonymous fn/15 in Ecto.Repo.Schema.do_insert/4
    (ecto 3.8.4) lib/ecto/repo/schema.ex:269: Ecto.Repo.Schema.insert!/4
    (elixir 1.13.4) lib/code.ex:1183: Code.require_file/2
    (mix 1.13.4) lib/mix/tasks/run.ex:146: Mix.Tasks.Run.run/5
nelsonic commented 2 years ago

Thanks to @SimonLab this was just a matter of MIX_ENV=dev mix compile --f and then it worked ✅

nelsonic commented 2 years ago

Reading: https://tailwindcomponents.com/component/todo-list-app

nelsonic commented 2 years ago

This looks like a good starting point:

<div class="h-100 w-full flex items-center justify-center bg-teal-lightest font-sans">
    <div class="bg-white rounded shadow p-6 m-4 w-full lg:w-3/4 lg:max-w-lg">
        <div class="mb-4">
            <h1 class="text-grey-darkest">Todo List</h1>
            <div class="flex mt-4">
                <input class="shadow appearance-none border rounded w-full py-2 px-3 mr-4 text-grey-darker" placeholder="Add Todo">
                <button class="flex-no-shrink p-2 border-2 rounded text-teal border-teal hover:text-white hover:bg-teal">Add</button>
            </div>
        </div>
        <div>
            <div class="flex mb-4 items-center">
                <p class="w-full text-grey-darkest">Add another component to Tailwind Components</p>
                <button class="flex-no-shrink p-2 ml-4 mr-2 border-2 rounded hover:text-white text-green border-green hover:bg-green">Done</button>
                <button class="flex-no-shrink p-2 ml-2 border-2 rounded text-red border-red hover:text-white hover:bg-red">Remove</button>
            </div>
            <div class="flex mb-4 items-center">
                <p class="w-full line-through text-green">Submit Todo App Component to Tailwind Components</p>
                <button class="flex-no-shrink p-2 ml-4 mr-2 border-2 rounded hover:text-white text-grey border-grey hover:bg-grey">Not Done</button>
                <button class="flex-no-shrink p-2 ml-2 border-2 rounded text-red border-red hover:text-white hover:bg-red">Remove</button>
            </div>
        </div>
    </div>
</div>

But we need to adapt it quite a lot. 💭

nelsonic commented 2 years ago

@SimonLab as discussed on our pairing session, the functionality of the initial item.create, toggle & delete is working:

Screenshot 2022-07-06 at 12 05 02

But for some reason the Tailwind is not working on the inner content ... 🤦‍♂️

Reading: https://elixirforum.com/t/tailwind-not-working-in-production-no-styles-just-plain-html/45192/3

nelsonic commented 2 years ago

If we put the Todo List sample code in the lib/app_web/templates/layout/root.html.heex it kinda works:

image

But still none of the background colors etc. work! 😞

nelsonic commented 2 years ago

This is what it's meant to look like:

image
nelsonic commented 2 years ago

Looks great, right? 🙄 image

More tomorrow. 🤞

Making progress ... image

nelsonic commented 2 years ago

Making progress thanks @RobStallion's Flex Skills 🥷

image

Ref: https://flexboxfroggy.com 😮

nelsonic commented 2 years ago

Obvs nowhere near "done". But getting there slowly ... 👨‍💻 ⏳

nelsonic commented 2 years ago

Minor progress:

image

nelsonic commented 2 years ago

Annoying side quest: https://github.com/dwyl/app-mvp-phoenix/issues/92 🙄

nelsonic commented 2 years ago

Very close to getting the timer working ... 🤞

image

nelsonic commented 2 years ago

Timer works but is a "hack" based on the fact that each time a timer is tarted a new timer is created. This is the desired behaviour. mvp-timer-working But it will not work if a new client is signed into the App once a timer is running. So we will need to re-work this soon.

nelsonic commented 2 years ago

Removed status schema from project:

image

No associations. Still does exactly what we need it to. Muuuuch simpler! 🙌

nelsonic commented 2 years ago

When I want to understand the [relational] data I always reach for SQL not Ecto. e.g:

    SELECT DISTINCT ON (i.id)
      i.id, i.text, i.status_code, i.person_id, t.start, t.end 
      FROM items i
    FULL JOIN timers as t 
      ON t.item_id = i.id
    WHERE i.person_id = $1
      AND i.status_code IS NOT NULL
      AND i.status_code != 6
    ORDER BY i.id DESC, 
      t.start DESC
    NULLS FIRST;
image

I know this is not idiomatic Ecto/Phoenix. So people in the Phoenix community may frown upon it ... 🙄 But at least 2 orders of magnitude more people know SQL than Ecto. https://github.com/dwyl/learn-postgresql#sql-is-everywhere So this query will be familiar to way more people.

Phoenix prints the following debug statement in the terminal when this query is executed:

[debug] QUERY OK db=2.2ms queue=3.7ms idle=351.7ms
SELECT DISTINCT ON (i.id)
  i.id, i.text, i.status_code, i.person_id, t.start, t.end
  FROM items i
FULL JOIN timers as t
  ON t.item_id = i.id
WHERE i.person_id = $1
  AND i.status_code IS NOT NULL
  AND i.status_code != 6
ORDER BY i.id DESC,
  t.start DESC
NULLS FIRST;
 [1]
["id", "text", "status_code", "person_id", "start", "end"]
[7, "This is my awesome item in browser", 2, 1, ~N[2022-07-14 16:42:45.000000],
 ~N[2022-07-14 16:42:48.000000]]

It's immediately clear to me what is going on here and I can easily debug it when it goes "wrong". In Phoenix, the query takes 2.2ms to execute [without any optimisation] on localhost. Fairly certain it will be a lot faster if we add the appropriate indexes. And in prod we could cache the results of the queries for sub-millisecond response times. (_The network is always the bottleneck...)

Not saying I don't want to use Ecto and associations dwyl/app-mvp-phoenix#94 ... Just that adding a lot of code to create those associations needs to offer a substantial a performance/maintainability benefit. I'm a lot more confident that I can optimise Postgres (when the time comes; definitely not now...!)

This "raw" SQL can easily be copy-pasted into the Postgres editor to debug/modify and optimise it.

image

Nothing to "pre-load", no additional boilerplate cluttering the code/schema in various places. I would sooner have SQL queries that I can immediately see what they do. It's like the difference between declarative in-line CSS in Markup [Tailwind] vs. having CSS spread across several files.

I really like how IHP promotes the use of plain SQL over higher-level abstractions on the persistance layer: https://ihp.digitallyinduced.com/Features image I wish Phoenix had taken that approach instead of clumsily abstracting queries in Ecto. I can see why Ecto is this way with José coming from Rails ... 💭 There are definitely advantages to having a "layer" there, e.g. it permits the use of Fields for transparent encryption/decryption.

Anyway ... this is what we're doing in the MVP so that more people can understand it without knowing Ecto ... ⏳

Anyone who knows even basic SQL (e.g. anyone who did a basic Data Science course) will immediately understand this with zero learning curve.

nelsonic commented 2 years ago

Dogfooding: 😜 image

nelsonic commented 2 years ago

Added icons to the buttons: dwyl/app-mvp-phoenix#101

image

nelsonic commented 2 years ago

been neglecting the dev log in the README.md these last few days as I learn Tailwind (flex!) ... ⏳ 3 4 more task to do:

  1. #102
  2. #103
    1. #106
  3. dwyl/app-mvp-phoenix#104

Once those are done I plan to dedicate T4h to updating the README.md then will assign for review/feedback. 🤞

nelsonic commented 2 years ago

GIF at the end of today:

mvp-edit-item-timer-displays-when-stopped

Note: 3x speed cause ... ⏳ == 💸

nelsonic commented 2 years ago

Clicking/tapping the checkbox to mark an item as "done" will now end the active timer:

mvp-showing-checkbox-ends-timer

UX enhancement that made sense to me. see: dwyl/app-mvp-phoenix#107

nelsonic commented 2 years ago

Auth implemented: 🚀

mvp-auth-implemented-2x-speed-full-quality

nelsonic commented 2 years ago

This MVP is now feature complete! ref: https://github.com/dwyl/app/issues/265 🎉 Just need to deploy to Fly.io #109 🚀 Then need to go through the README.md and update all the instructions. 📝 ⏳

nelsonic commented 2 years ago

Deployed to Fly.io https://mvp.fly.dev/ 🚀 image

Will make a decent GIF after lunch. 👌

nelsonic commented 2 years ago

GIF: mvp-fly-auth

nelsonic commented 2 years ago

Been re-re-re-re-re-reading the README.md ... 👀 and decided to take a quick detour to rename timer.end to timer.stop: https://github.com/dwyl/app-mvp/issues/114 🔚 BRB! 🧑‍💻 ⏳