[WIP] Feature/enable bookmarking by WanjikuMac · Pull Request #39 · LittleKidogo/quick_bytes_system

WanjikuMac · 2018-10-21T17:19:50Z

Fixes
#33

zacck-zz

Hey @WanjikuMac good work on this PR and on building that Macro glad to see you could use it ... Just a few changes to get this to fly nice!

zacck-zz · 2018-10-21T18:01:43Z

lib/qb_backend/posts/bookmark.ex

+  @moduledoc """
+  This module defines the schema for the bookmark section
+  """
+  use QbBackend.Utils.Schema


ahahah I like this!

zacck-zz · 2018-10-21T18:02:50Z

lib/qb_backend/posts/bookmark.ex

+    Posts.Bookmark
+  }
+
+  schema "bookmarks" do


@WanjikuMac could you perhaps consider giving a bookmark a name so maybe a user can have many bookmarks and the ability to choose from one

By bookmark name do you mean e.g bookmark 1? Otherwise the bookmark name is the name of the manual. Like when you book mark a post you retrieve it by the name of the manual

@WanjikuMac no I mean the bookmark should have a field for the name of the bookmark, Consider the case where one person wants many bookmarks for example. I could have a bookmark that stores all the Yoga stuff I want to read and I could name it my yoga reads and I could have a bookmark that stores all the Business stuff I want to read and this one I could call kazi does this make sense?

@zacck Aaah, I see.

@WanjikuMac no I mean the bookmark should have a field for the name of the bookmark, Consider the case where one person wants many bookmarks for example. I could have a bookmark that stores all the Yoga stuff I want to read and I could name it my yoga reads and I could have a bookmark that stores all the Business stuff I want to read and this one I could call kazi does this make sense?

zacck-zz · 2018-10-21T18:07:03Z

lib/qb_backend/posts/bookmark.ex

+  def add_manual_to_bookmark(%Bookmark{id: _id, manuals: manuals} = bookmark, %Manual{} = manual) do
+    bookmark
+    |> changeset(%{})
+    |> put_assoc(:manuals, manuals ++ [manual])


appending to lists is very expensive since the compiler has to walk through all the items to add one perhaps consider prepending to the list like this [manual] ++ manuals see here https://elixirforum.com/t/when-would-you-need-to-append-to-an-elixir-list-over-prepending/12908

zacck-zz · 2018-10-21T18:09:35Z

lib/qb_backend/posts/manual.ex

+  This function takes a bookmark and a manual and creates an association between them
+  """
+  @spec add_to_bookmark(Manual.t(), Bookmark.t()) :: {:ok, Manual.t()} | {:error, Ecto.Changeset.t()}
+  def add_to_bookmark(%Manual{} = manual, %Bookmark{} = bookmark) do


I am not so sure that we need the functions in the Manual for adding a manual to a bookmark, I think we should only have the ones in the bookmark since the bookmark is the item that owns in this relationship

@WanjikuMac I thought about this situation and I think I changed my mind, I realized that we may need to keep these functions however we will need to convert the relationship to a many-to-many since it is very likely that down the line we will need to answer these two questions

How many manuals are in a bookmark

In how many bookmarks is a manual

These two would need us to have and maintain a bidirectional relationship I think

zacck-zz · 2018-10-22T03:27:41Z

@WanjikuMac hmmmm so if I as a profile own a bookmark I will expect it to have a name like I could have a Yoga bookmark and I could put yoga manuals I am interested in that bookmark but later down the line I may want a business bookmark so I can put the business articles I am intrested in it there ... do you see

zacck-zz

hey @WanjikuMac some little pushes here and there and this should be good to fly!

zacck-zz · 2018-10-22T17:52:51Z

priv/repo/migrations/20181022100328_associate_manuals_to_bookmark.exs

+      add :bookmark_id, references(:bookmarks, type: :binary_id)
+    end
+
+    create index(:manuals, [:bookmark_id])


@WanjikuMac we likely don't need this index any more since I think this is in the join table

Yea the tests are passing without it.

zacck-zz · 2018-10-22T17:53:34Z

test/lib/qb_backend/posts/bookmark_test.exs

+    }
+    @valid_bookmark %{}
+  describe "Bookmark" do
+    test "Enable a user to create a bookmark" do


@WanjikuMac this test actually just test Drives the bookmark changeset

@zacck This checks if a bookmark is actually created.

coveralls · 2018-10-22T20:11:21Z

Pull Request Test Coverage Report for Build 167

9 of 10 (90.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.7%) to 83.654%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/qb_backend/posts/manual.ex	0	1	0.0%

Totals
Change from base Build 152:	0.7%
Covered Lines:	87
Relevant Lines:	104

💛 - Coveralls

zacck-zz

@WanjikuMac we need to change our associations' strategy, otherwise coming along very well indeed

zacck-zz · 2018-10-23T01:59:43Z

lib/qb_backend/posts/manual.ex

+  def remove_manual_from_bookmark(%Manual{bookmarks: _bookmark} = manual) do
+    manual
+    |> changeset(%{})
+    |> put_change(:bookmark_id, nil)


@WanjikuMac this line isn't relevant

zacck-zz · 2018-10-23T02:00:02Z

lib/qb_backend/posts/manual.ex

+    manual
+    |> changeset(%{})
+    |> put_change(:bookmark_id, nil)
+    |> put_change(:bookmark, nil)


this line is also not relevant

zacck-zz · 2018-10-23T02:01:13Z

lib/qb_backend/posts/manual.ex

+    manual
+    |> changeset(%{})
+    |> put_change(:bookmark_id, nil)
+    |> put_change(:bookmark, nil)


This line is not relevant, we are going to need to find another way of building this associations as this strategy doesn't work for many to many associations

@zacck can we have this instead put_assoc(:bookmark, manuals) ?

erm can you check how the associations work on spender between an Item and a Logsection

@zacck @br4inii I think we don't need the add manual to bookmark on the manual.ex since it's already captured on the bookmark.ex file. Plus the manual.ex file can already create a manual for us... What are your thoughts?

@WanjikuMac however way works for you and is most efficient ... remember deleting code is better than adding

zacck-zz

Hey @WanjikuMac just a few changes to push this over the hill

zacck-zz · 2018-11-05T06:29:19Z

lib/qb_backend/posts/posts.ex

  end
+
+  @doc """
+  This function actually adds the manual struct to the bookmark


@WanjikuMac please document your parameters for this function

zacck-zz · 2018-11-05T06:29:38Z

lib/qb_backend/posts/posts.ex


  @doc """
-  this function delets an image struct from a manual
+  this function deletes an image struct from a manual


@WanjikuMac this needs parameter documentation too

zacck-zz

@WanjikuMac this is looking good so far

WanjikuMac added 7 commits October 21, 2018 20:06

added bookmark migration

51ea5ad

added the bookmark schema

ea312f3

added bookmark tests

7412139

added hooks

2e99839

added relationship to bookmark

477fdcb

added association to bookmark

117765a

added bookmark factory

5306817

WanjikuMac requested review from r-coh and zacck-zz October 21, 2018 17:19

zacck-zz requested changes Oct 21, 2018

View reviewed changes

WanjikuMac added 5 commits October 22, 2018 20:46

changed associaton to manuals

fb94cf6

changed type of association bookmark

69f105c

removed the alter manuals table

d85cda6

This is the alter manuals table

52344ef

Migration to associate bookmarks to manuals

64091c1

zacck-zz requested changes Oct 22, 2018

View reviewed changes

removed the create index for bookmark id

0e8b97c

zacck-zz requested changes Oct 23, 2018

View reviewed changes

WanjikuMac added 3 commits November 4, 2018 13:04

added association between the bookmark and a manual

619d8ac

added a resolver to add a manual to a bookmark

70caede

resolver test to add a manual to bookmarks

1359fd7

zacck-zz requested changes Nov 5, 2018

View reviewed changes

added parameter documentation

985eff7

zacck-zz approved these changes Nov 5, 2018

View reviewed changes

Conversation

WanjikuMac commented Oct 21, 2018

Uh oh!

zacck-zz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacck-zz commented Oct 22, 2018

Uh oh!

zacck-zz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 167

💛 - Coveralls

Uh oh!

zacck-zz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacck-zz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacck-zz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

coveralls commented Oct 22, 2018 •

edited

Loading