Add Tag on task using Enter #389

panoramix360 · 2023-07-04T19:00:28Z

This PR adds the functionality to select Tags when the user presses Enter while searching for tags.

It will always pick the first tag that appears on the list, if there are no tags, nothing will be added.

The behavior is detailed on the issue.

I tried to make it simple.

Please, let me know if you guys think it's needed any changes.

I didn't like the way I managed to empty the field after the user press Enter but I tried using LiveView <.input> with no success, if you guys have any other ideas let me know, I'd love to learn since I'm new to Phoenix/LiveView.

:D

…o the selected tags when user presses enter

panoramix360 · 2023-07-07T15:42:35Z

hey @nelsonic, I notice that the Github Checks didn't pass, is there something on my end that I need to fix?

LuchoTurtle · 2023-07-07T16:52:29Z

Hey @panoramix360 .
Thanks for the PR! Run mix format to fix the Build and Test workflow. We have it setup so the code is correctly formatted before being pushed into the repo.
Thanks!

panoramix360 · 2023-07-07T17:04:37Z

thanks @LuchoTurtle!

Done!

LuchoTurtle · 2023-07-07T17:15:28Z

@nelsonic we're getting an error with the deploy review app script.

https://github.com/dwyl/mvp/actions/runs/5488807164/jobs/10002305167

Review App Script
Error: No access token available. Please login with 'flyctl auth login'

Have you stumbled upon this error before?

LuchoTurtle · 2023-07-07T17:16:52Z

We need to set FLY_ACCESS_TOKEN in order for this to work.

https://fly.io/docs/flyctl/integrating/

panoramix360 · 2023-07-10T18:35:27Z

@LuchoTurtle, is this something that I should do on the PR?

LuchoTurtle · 2023-07-10T18:45:36Z

@panoramix360 the issue is on our end, so please just give us some time so get it sorted and we'll get right back to ya with this PR and get it merged :).

Meanwhile, we're getting this warning with the function you've added.

clauses with the same name and arity (number of arguments) should be grouped together, "def handle_event/3" was previously defined (lib/app_web/live/app_live.ex:45)

I'll create a review and request a small change from you :)

lib/app_web/live/app_live.ex

nelsonic · 2023-07-11T08:41:31Z

@panoramix360 I think the CI doesn't run because you created a fork instead of a branch. 💭
We need to fix that on our side as @LuchoTurtle says. 🔧
You have an invite for write access to the repo for any future contributions. ✍️

nelsonic · 2023-07-11T08:56:59Z

I checked out @panoramix360 branch:

git clone [email protected]:panoramix360/mvp.git && cd mvp
git checkout 'feature/add-task-with-tag-when-enter'
cp ../.env .
source .env
mix deps.get
mix c

Got:

Finished in 1.2 seconds (1.1s async, 0.1s sync)
118 tests, 0 failures

Randomized with seed 640070
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/api/item.ex                               218       56        0
100.0% lib/api/tag.ex                                101       24        0
100.0% lib/api/timer.ex                              152       40        0
100.0% lib/app/color.ex                               90        1        0
100.0% lib/app/item.ex                               377       53        0
100.0% lib/app/item_tag.ex                            12        1        0
100.0% lib/app/tag.ex                                108       18        0
100.0% lib/app/timer.ex                              481       90        0
100.0% lib/app_web/controllers/auth_controller.       26        4        0
100.0% lib/app_web/controllers/init_controller.       41        6        0
100.0% lib/app_web/controllers/tag_controller.e       77       25        0
 94.6% lib/app_web/live/app_live.ex                  441      130        7
100.0% lib/app_web/live/stats_live.ex                 77       21        0
100.0% lib/app_web/router.ex                          49        9        0
100.0% lib/app_web/views/error_view.ex                59       12        0
  0.0% lib/app_web/views/profile_view.ex               3        0        0
  0.0% lib/app_web/views/tag_view.ex                   3        0        0
[TOTAL]  98.6%
----------------
Generating report...

i.e. test coverage decreased by 1.4% due to the new code.

Reviewing the HTML coverage report we see that the lines that are not covered by tests are exactly the lines that are being added by this PR:

Sadly, CI doesn't pass because @panoramix360 created a fork.
This is a problem with how GitHub handles Environment Variables. 🤦‍♂️

The only way we can make the CI pass is to make the AUTH_API_KEY public. 🔓
Or for @panoramix360 to re-create these changes as a branch on the @dwyl source version.
I would prefer to resolve the Environment Variables once and for all to avoid wasting contributor time in the future.

nelsonic · 2023-07-11T08:59:08Z

The CI error is in the Build and Test phase: https://github.com/dwyl/mvp/actions/runs/5514171282/jobs/10060694853?pr=389#step:8:66

This is because GitHub does not give access to AUTH_API_KEY to PRs based on Forks.
This is a super annoying feature of GitHub CI we cannot circumvent. 😢

nelsonic · 2023-07-11T09:01:29Z

To write a test for this new functionality we need to emit a keydown event:
https://hexdocs.pm/phoenix_live_view/Phoenix.LiveViewTest.html#render_keydown/1
I'm happy to add this test if you're short on time @panoramix360

nelsonic · 2023-07-11T09:03:58Z

As for the failing Review App Job: https://github.com/dwyl/mvp/actions/runs/5488807164/jobs/10002305167#step:5:20

This is exactly the same issue described above, GitHub is blocking access to the FLY_API_TOKEN environment variable because the PR was created from a fork.

Once this is merged into main the deploy to fly.io will work fine as it has in the past.

nelsonic · 2023-07-11T09:10:46Z

Sadly, we still have the regression that we cannot add tags dynamically as outlined in: #221 (comment)

But if I manually add a couple of tags on my localhost:

And then test the feature that @panoramix360 is proposing:

I cannot get the desired behaviour described by @iteles in #308 (comment) 💭

nelsonic · 2023-07-11T09:11:44Z

@iteles could you please help us with the requirements so that we can review this PR ASAP. 🙏

nelsonic · 2023-07-11T09:12:40Z

@panoramix360 meanwhile if you can add a test for the Enter keypress event I will git pull from your branch. 🙏

panoramix360 · 2023-07-11T13:59:54Z

hey! Thank you for inviting me to the repo, if you prefer I can create the branch on the repo and delete the fork, but as you mentioned, it's good to have that fixed for future contributions.

I can create the tests, no problem, sorry for not creating it earlier, I didn't see that the project had tests.

Regarding the behavior, what happened when you pressed enter? It didn't work?

panoramix360 · 2023-07-12T13:18:31Z

hey @nelsonic! do you have any tips on how to test it? I'm new to Phoenix LiveView and I'm thinking of using this one to test it:
https://hexdocs.pm/phoenix_live_view/Phoenix.LiveViewTest.html#render_keydown/1

If you have any ideas or directions to it, it'd be helpful!

panoramix360 · 2023-07-12T14:14:54Z

EDIT: It's tested now, scroll through the next comment

I'm trying to do something like that:

Create a new tag on the database (Tag.create_tag)
Then I use the render_keydown to fire the Enter event
Then I use the render_submit to create a new item on the database
Then check if the item is created with the first tag

This is my code:

test "select tag when enter pressed", %{conn: conn} do
    {:ok, view, html} = live(conn, "/")

    {:ok, _tag1} =
      Tag.create_tag(%{person_id: 0, text: "tag1", color: "#FCA5A5"})

    {:ok, _tag2} =
      Tag.create_tag(%{person_id: 0, text: "enter_tag", color: "#FCA5A5"})

    assert render_keydown(view, "add-first-tag", %{
             "key" => "Enter",
             "value" => "enter_tag"
           })

    assert render_submit(view, :create, %{text: "tag enter pressed"})

    IO.inspect(Tag.list_person_tags(0))
    IO.inspect(Item.get_item!(2).tags)
  end

(Ignore the IO.inspect, is just for checking)

It seems that the render_keydown is not finding any tags created, which is strange since I was creating before it.

I believe I'm missing something

panoramix360 · 2023-07-12T14:39:18Z

UPDATE:

I managed to test it right now :D

@nelsonic review it when you can! thank you for your link, it was helpful!

The test coverage is now 99.8%, feel free to let me know to add more things if needed.

LuchoTurtle · 2023-07-12T16:15:27Z

Hey @panoramix360

Awesome job on proactively working on this, it's mighty appreciated! I've pulled the remote from your forked repo and ran your branch locally and it seems to work as you've described.

Screen.Recording.2023-07-12.at.17.07.13.mov

I've seen you've mentioned it only toggles the tag on top of the list. Although it would make more sense to get the first tag not selected, I'm aware that you've tried to make it simple.

When I run the tests mix c, they seem to pass as well:

Finished in 1.1 seconds (1.0s async, 0.08s sync)
121 tests, 0 failures

Randomized with seed 879703
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/api/item.ex                               218       56        0
100.0% lib/api/tag.ex                                101       24        0
100.0% lib/api/timer.ex                              152       40        0
100.0% lib/app/color.ex                               90        1        0
100.0% lib/app/item.ex                               377       53        0
100.0% lib/app/item_tag.ex                            12        1        0
100.0% lib/app/tag.ex                                108       18        0
100.0% lib/app/timer.ex                              481       90        0
100.0% lib/app_web/controllers/auth_controller.       26        4        0
100.0% lib/app_web/controllers/init_controller.       41        6        0
100.0% lib/app_web/controllers/tag_controller.e       77       25        0
100.0% lib/app_web/live/app_live.ex                  441      130        0
100.0% lib/app_web/live/stats_live.ex                 77       21        0
100.0% lib/app_web/router.ex                          49        9        0
100.0% lib/app_web/views/error_view.ex                59       12        0
  0.0% lib/app_web/views/profile_view.ex               3        0        0
  0.0% lib/app_web/views/tag_view.ex                   3        0        0
[TOTAL] 100.0%

Thanks for getting the test coverage back to 100%. I'm glad you've worked more on them (saw your latest commits), the assertions make more sense now and actually test if the tag is selected.

I'm going to make a small comment on the review shortly :)

test/app_web/live/app_live_test.exs

… same on db

LuchoTurtle

Looks good!
I'll let @nelsonic have a final say.

Thanks for the PR 😄

panoramix360 · 2023-07-13T13:44:15Z

No worries! I'll look through the next issue to complete, if you have any suggestions let me know!

nelsonic

@panoramix360 great contribution. 🎉
Thanks for adding the tests. ✅

Confirmed on localhost:

git pull
mix c

Finished in 1.3 seconds (1.2s async, 0.08s sync)
121 tests, 0 failures

Randomized with seed 811468
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/api/item.ex                               218       56        0
100.0% lib/api/tag.ex                                101       24        0
100.0% lib/api/timer.ex                              152       40        0
100.0% lib/app/color.ex                               90        1        0
100.0% lib/app/item.ex                               377       53        0
100.0% lib/app/item_tag.ex                            12        1        0
100.0% lib/app/tag.ex                                108       18        0
100.0% lib/app/timer.ex                              481       90        0
100.0% lib/app_web/controllers/auth_controller.       26        4        0
100.0% lib/app_web/controllers/init_controller.       41        6        0
100.0% lib/app_web/controllers/tag_controller.e       77       25        0
100.0% lib/app_web/live/app_live.ex                  441      130        0
100.0% lib/app_web/live/stats_live.ex                 77       21        0
100.0% lib/app_web/router.ex                          49        9        0
100.0% lib/app_web/views/error_view.ex                59       12        0
  0.0% lib/app_web/views/profile_view.ex               3        0        0
  0.0% lib/app_web/views/tag_view.ex                   3        0        0
[TOTAL] 100.0%
----------------
Generating report...
Saved to: cover/

mix s

@LuchoTurtle thanks for reviewing + trouble-shooting. ❤️

panoramix360 added 4 commits July 4, 2023 15:23

feat: add new keydown event to html and prevent enter to submit form

2ef04ea

feat: creates new function that toggle tag and adding the first tag t…

893275b

…o the selected tags when user presses enter

feat: cleans the field when user pressed enter inside the input text

d4549d7

feat: refactor to deal with empty case

78579fa

panoramix360 mentioned this pull request Jul 4, 2023

Clicking enter when in the tag field should add the tag to the timer #308

Closed

chore: mix format

b31e520

LuchoTurtle self-assigned this Jul 10, 2023

LuchoTurtle added enhancement New feature or enhancement of existing functionality good first issue Good for newcomers documentation Improvements or additions to documentation labels Jul 10, 2023

LuchoTurtle requested changes Jul 10, 2023

View reviewed changes

lib/app_web/live/app_live.ex Outdated Show resolved Hide resolved

panoramix360 mentioned this pull request Jul 10, 2023

Task: MVP UX/UI Test + Feedback! 🙏 #140

Open

4 tasks

chore: moving defp function to appropriate place

3680690

nelsonic added the in-progress An issue or pull request that is being worked on by the assigned person label Jul 11, 2023

nelsonic assigned panoramix360 Jul 11, 2023

nelsonic assigned iteles and unassigned LuchoTurtle Jul 11, 2023

nelsonic added help wanted If you can help make progress with this issue, please comment! chore a tedious but necessary task often paying technical debt elixir Pull requests that update Elixir code labels Jul 11, 2023

feat: adding tests to the add first tag when enter pressed

cda7a36

panoramix360 requested a review from LuchoTurtle July 12, 2023 15:03

feat: adding another test for better coverage

1bf3eb2

LuchoTurtle requested changes Jul 12, 2023

View reviewed changes

test/app_web/live/app_live_test.exs Show resolved Hide resolved

feat: incrementing select tag test to see if the tag was actually the…

43d39df

… same on db

panoramix360 requested a review from LuchoTurtle July 12, 2023 21:10

LuchoTurtle approved these changes Jul 13, 2023

View reviewed changes

LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Jul 13, 2023

LuchoTurtle assigned nelsonic Jul 13, 2023

nelsonic approved these changes Jul 14, 2023

View reviewed changes

nelsonic merged commit 9c28ed8 into dwyl:main Jul 14, 2023

panoramix360 deleted the feature/add-task-with-tag-when-enter branch July 14, 2023 14:32

nelsonic mentioned this pull request Jul 18, 2023

[PR] Adding drag n drop and index-based reordering #345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tag on task using Enter #389

Add Tag on task using Enter #389

panoramix360 commented Jul 4, 2023 •

edited

Loading

panoramix360 commented Jul 7, 2023 •

edited

Loading

LuchoTurtle commented Jul 7, 2023

panoramix360 commented Jul 7, 2023

LuchoTurtle commented Jul 7, 2023

LuchoTurtle commented Jul 7, 2023

panoramix360 commented Jul 10, 2023

LuchoTurtle commented Jul 10, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

panoramix360 commented Jul 11, 2023

panoramix360 commented Jul 12, 2023

panoramix360 commented Jul 12, 2023 •

edited

Loading

panoramix360 commented Jul 12, 2023 •

edited

Loading

LuchoTurtle commented Jul 12, 2023

LuchoTurtle left a comment

panoramix360 commented Jul 13, 2023

nelsonic left a comment

Add Tag on task using Enter #389

Add Tag on task using Enter #389

Conversation

panoramix360 commented Jul 4, 2023 • edited Loading

panoramix360 commented Jul 7, 2023 • edited Loading

LuchoTurtle commented Jul 7, 2023

panoramix360 commented Jul 7, 2023

LuchoTurtle commented Jul 7, 2023

LuchoTurtle commented Jul 7, 2023

panoramix360 commented Jul 10, 2023

LuchoTurtle commented Jul 10, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

nelsonic commented Jul 11, 2023

panoramix360 commented Jul 11, 2023

panoramix360 commented Jul 12, 2023

panoramix360 commented Jul 12, 2023 • edited Loading

panoramix360 commented Jul 12, 2023 • edited Loading

LuchoTurtle commented Jul 12, 2023

LuchoTurtle left a comment

Choose a reason for hiding this comment

panoramix360 commented Jul 13, 2023

nelsonic left a comment

Choose a reason for hiding this comment

panoramix360 commented Jul 4, 2023 •

edited

Loading

panoramix360 commented Jul 7, 2023 •

edited

Loading

panoramix360 commented Jul 12, 2023 •

edited

Loading

panoramix360 commented Jul 12, 2023 •

edited

Loading