Bug: Item disappears after editing 🐞

[LuchoTurtle]

I don't know if this is strictly related to #417 but I've noticed that if I edit an item, after saving it, the item disappears.
Check the video below.

Screen.Recording.2023-09-09.at.01.53.12.mov

After refreshing, the edited item is shown normally.

[LuchoTurtle]

It seems that this line is returning the incomplete list.

mvp/lib/app/item.ex

Lines 217 to 218 in b8aee0e

	all_list = App.List.get_all_list_for_person(person_id)
	seq = App.List.get_list_seq(all_list)

[LuchoTurtle]

Everytime I edit an item, it seems it creates a null item with status 7 🤔

[LuchoTurtle]

If I delete every item and refresh the page, an empty one is created.

This is because in lib/app/list.ex creaates a new list (since it doesn't have any):

  def get_all_list_for_person(person_id) do
    # IO.inspect("get_all_list_for_person(person_id: #{person_id})")
    all_list = get_list_by_name!("all", person_id)

	# Creates list
    if all_list == nil do
      # doesn't exist, create it:
      {:ok, %{model: list}} =
        create_list(%{name: "all", person_id: person_id, status: 2})

      list
    else
      all_list
    end
  end

In turn, this calls the create_list/1 in lib/app/list.ex function.

  def create_list(attrs) do
    %List{}
    |> changeset(attrs)
    |> PaperTrail.insert()
  end

changeset/1 creates a new cid, apparently:

  def changeset(list, attrs) do
    list
    |> cast(attrs, [:name, :person_id, :seq, :sort, :status])
    |> validate_required([:name, :person_id])
    |> App.Cid.put_cid()
  end

I don't know why an item is created, since this obviously only leads with lists. :\

[LuchoTurtle]

Turns out this empty item is created on mount in app_live.ex.

mvp/lib/app_web/live/app_live.ex

Line 31 in b8aee0e

draft_item = Item.get_draft_item(person_id)

  def get_draft_item(person_id) do
    Repo.get_by(Item, status: 7, person_id: person_id) ||
      Repo.insert!(%Item{person_id: person_id, status: 7})
  end

Maybe this is intended or not but I feel like I'm digressing ☠️

[LuchoTurtle]

I think I found the reason why the item disappears after being edited. It's because after being edited, its cid changes but the seq attribute of the list it belongs to isn't properly updated to have this new cid appended.

Can't figure why it's properly appended if I refresh the page though :(

[LuchoTurtle]

Ok, I think I understand it now, but I have a few questions.

Why is this failing?

Every time an item is updated, Item.update_item/2 is called.

mvp/lib/app/item.ex

Lines 165 to 169 in b8aee0e

	def update_item(%Item{} = item, attrs) do
	item
	|> Item.changeset(attrs)
	|> PaperTrail.update(originator: %{id: Map.get(attrs, :person_id, 0)})
	end

Which, in turn, calls changeset/1, which changes the item's cid after updating.

mvp/lib/app/item.ex

Lines 25 to 30 in b8aee0e

	def changeset(item, attrs) do
	item
	|> cast(attrs, [:cid, :person_id, :status, :text])
	|> validate_required([:text, :person_id])
	|> App.Cid.put_cid()
	end

What's missing here is, inside update_item/2, we also update the item's list seq field with this new cid.

Why does the item show after refreshing the page?

Simple. Because, on mount/3, App.List.add_all_items_to_all_list_for_person_id(person_id) is called, which adds all items to the "all" list of the person, thus updating the seq field accordingly.

mvp/lib/app_web/live/app_live.ex

Lines 18 to 31 in b8aee0e

	def mount(_params, _session, socket) do
	# subscribe to the channel
	if connected?(socket), do: AppWeb.Endpoint.subscribe(@topic)
	AppWeb.Endpoint.subscribe(@stats_topic)
	person_id = Person.get_person_id(socket.assigns)
	# Create or Get the "all" list for the person_id
	all_list = App.List.get_all_list_for_person(person_id)
	# Temporary function to add All *existing* items to the "All" list:
	App.List.add_all_items_to_all_list_for_person_id(person_id)
	items = Item.items_with_timers(person_id)
	tags = Tag.list_person_tags(person_id)
	selected_tags = []
	draft_item = Item.get_draft_item(person_id)

OK, so just update the list seq and you're through 😐

It's not so simple. Here's what's happening:

• the seq field in the list to which the item belongs (in my tests, I'm always using the default "all" list), only gets the new cid appended. This means, the seq field will have cids that actually don't exist, because they've been overridden after the item being updated.

This doesn't really pose a problem, since cid has a very low probability of conflict. It's just a matter of optimization and saving space.

• In the relational diagram (check below), an item doesn't know which list it belongs to. The list "knows" because they have a seq field with cids separated by commas pertaining to the items. Since there is no association between tables, it's difficult to know inside Item.update_item/2 which list should be updated, because we can't possibly know that from the item.

I'm aware that list_items was a previous schema that got rightfully deleted (check #413). However, it made sense to delete it because it wasn't really a bridge table. If we use it as such, we can know which list and item belongs to and vice-versa.

Next steps

We know what we need to change. We need to update the list after the item is updated in Item.update_item/2.

  def update_item(%Item{} = item, attrs) do
    item
    |> Item.changeset(attrs)
    |> PaperTrail.update(originator: %{id: Map.get(attrs, :person_id, 0)})


	# We need to update the list the `item` belongs to 
  end

However, to know which list the item belongs to, we may have to change how the database is laid out.
A simpler alternative is passing the id of the list to Item.update_item/2 and work from there. But I don't know if this topic is a side-effect of an underlying issue or if I'm overreacting 🤷

@nelsonic opinions?

[nelsonic]

@LuchoTurtle Thanks for opening this issue. 📥
I was hoping someone else would use the MVP enough to spot it. 🤞
Think of it as a "test" to see who's paying attention. 😉
Your understanding is correct. ✅

The reason for this issue is simple: the item.cid is being updated by:

mvp/lib/app/item.ex

Lines 122 to 126 in 68f5562

	def update_item(%Item{} = item, attrs) do
	item
	|> Item.changeset(attrs)
	|> Repo.update()
	end

Which invokes changeset/2:

mvp/lib/app/item.ex

Lines 25 to 30 in da1cc04

	def changeset(item, attrs) do
	item
	|> cast(attrs, [:cid, :person_id, :status, :text])
	|> validate_required([:text, :person_id])
	|> App.Cid.put_cid()
	end

specifically the line:

mvp/lib/app/item.ex

Line 29 in da1cc04

|> App.Cid.put_cid()

which invokes:

https://github.com/dwyl/mvp/blob/da1cc04d74f670e69e4ba58eebfc71a24fc523ec/lib/app/cid.ex#L10C7-L24

The problem is: when the item.text is updated the cid (hash of the Map) updates.
This is technically an (undocumented) feature not a bug.
However if we are basing our lists on having a specific item.cid in the list.seq then the item.cid should not update!

To resolve this issue is very easy.
We simply need a changeset_update function that is called in the update_item/2 function:

  # Update an item without changing the cid ref: #418
  def changeset_update(item, attrs) do
    item
    |> cast(attrs, [:cid, :person_id, :status, :text])
    |> validate_required([:cid, :text, :person_id])
  end

Already fixed by:

mvp/lib/app/item.ex

Lines 43 to 48 in 8acf8c0

	# Update an item without changing the cid ref: #418
	def changeset_update(item, attrs) do
	item
	|> cast(attrs, [:cid, :person_id, :status, :text])
	|> validate_required([:cid, :text, :person_id])
	end

Included in the PR I'm updating: #165 🧑‍💻 ⏳

P.S: "opinions" never. Only logic. The logic in this was bad for the long-term of lists. So we fixed it. ✅
Thanks again for opening the issue. 🙏