Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PR] Adding liveview version without pushing to S3 from client #87

Merged
merged 24 commits into from
Jul 19, 2023

Conversation

LuchoTurtle
Copy link
Member

@LuchoTurtle LuchoTurtle commented Jul 4, 2023

closes #62

This creates a new liveview page that doesn't upload the images to the S3 bucket through the client.

@LuchoTurtle LuchoTurtle added enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person elixir Pull requests that update Elixir code documentation Improvements or additions to documentation labels Jul 4, 2023
@LuchoTurtle LuchoTurtle self-assigned this Jul 4, 2023
@LuchoTurtle
Copy link
Member Author

I'm having a bit of trouble trying to debug the cause of refreshing the page after an image is locally uploaded.
The file, after it's selected in the input, is being correctly materialized locally.

image

However, this triggers a refresh of the page, which doesn't allow me to "see" the locally uploaded files (which are meant to later be uploaded to S3).

I initially thought because of the LiveReload library triggering the asset_change event which results in the refreshing of the page -> https://shankardevy.com/code/phoenix-live-reload/.

However, this doesn't seem to be the case. The code is configured correctly, as it's happening in the original LiveView.

I'm trying to assume this is an issue with form submitting and I'm trying to get preventDefault() to be invoked so this page refresh doesn't occur.

@nelsonic
Copy link
Member

nelsonic commented Jul 6, 2023

@LuchoTurtle did you figure it out? You've pushed quite a few more commits since the comment. If you're still stuck, ping me on Signal and we can Zoom-pair on it. 🧑‍💻

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Jul 6, 2023

Yeah, I've figured it out. This was because the asset_change event that was being created was prompted by LiveReload. I've changed the configuration of the folder and what LiveView watches so it reloads.

For the use case, I need to upload the files locally with auto-upload and then prompt the person to upload the file to S3.

Unfortunately, there is not a discernable way of getting the file's contents when the person inputs the file, so we ought to upload locally before uploading to S3 by using the upload/1 function you've implemented.

I've added testing and error handling but I still need to finish the documentation.

@LuchoTurtle LuchoTurtle marked this pull request as ready for review July 10, 2023 18:25
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #87 (63e54ef) into main (66a8e0f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #87   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         8    +1     
  Lines           95       127   +32     
=========================================
+ Hits            95       127   +32     
Impacted Files Coverage Δ
lib/app_web/controllers/page_controller.ex 100.00% <100.00%> (ø)
lib/app_web/live/imgup_no_client_live.ex 100.00% <100.00%> (ø)
lib/app_web/router.ex 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

# Conflicts:
#	test/app_web/api_test.exs
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jul 17, 2023
@LuchoTurtle 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 17, 2023
@LuchoTurtle
Copy link
Member Author

This should be ready for review.
I've updated the code without mocks and have updated the branch so it doesn't have merge conflicts with #86 .

README.md Outdated Show resolved Hide resolved
LuchoTurtle and others added 2 commits July 17, 2023 16:07
@nelsonic
Copy link
Member

Tried this on my localhost:

git checkout 'clientless-#62'
git pull
mix deps.get
mix s

Visit http://localhost:4000/liveview_clientless and attempt to upload a recent image file and got:
image

image

Stack trace:

[debug] HANDLE EVENT "upload_to_s3" in AppWeb.ImgupNoClientLive
  Parameters: %{"uuid" => "205fc761-9644-422a-9007-7ca5f2fdeb24", "value" => ""}
[error] There was a problem uploading the file to S3.
[error] ** (Protocol.UndefinedError) protocol Enumerable not implemented for nil of type Atom
    (elixir 1.14.3) lib/enum.ex:1: Enumerable.impl_for!/1
    (elixir 1.14.3) lib/enum.ex:166: Enumerable.reduce/3
    (elixir 1.14.3) lib/enum.ex:4307: Enum.reverse/1
    (elixir 1.14.3) lib/enum.ex:3646: Enum.to_list/1
    (elixir 1.14.3) lib/map.ex:228: Map.new_from_enum/1
    (ex_aws 2.4.3) lib/ex_aws/config.ex:66: ExAws.Config.new/2
    (ex_aws 2.4.3) lib/ex_aws.ex:73: ExAws.request/2
    (app 1.0.0) lib/app/upload.ex:74: App.Upload.upload_file_to_s3/3
    (app 1.0.0) lib/app/upload.ex:26: App.Upload.upload/1
    (app 1.0.0) lib/app_web/live/imgup_no_client_live.ex:85: AppWeb.ImgupNoClientLive.handle_event/3
    (phoenix_live_view 0.19.4) lib/phoenix_live_view/channel.ex:401: anonymous fn/3 in Phoenix.LiveView.Channel.view_handle_event/3
    (telemetry 1.2.1) /Users/n/code/img/app/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
    (phoenix_live_view 0.19.4) lib/phoenix_live_view/channel.ex:221: Phoenix.LiveView.Channel.handle_info/2
    (stdlib 4.2) gen_server.erl:1123: :gen_server.try_dispatch/4
    (stdlib 4.2) gen_server.erl:1200: :gen_server.handle_msg/6
    (stdlib 4.2) proc_lib.erl:240: :proc_lib.init_p_do_apply/3

@nelsonic
Copy link
Member

Happy to show you on my computer when you're available. e.g. standup tomorrow morning. 👍

@nelsonic nelsonic assigned LuchoTurtle and unassigned nelsonic Jul 18, 2023
@nelsonic nelsonic added user-feedback technical A technical issue that requires understanding of the code, infrastructure or dependencies and removed awaiting-review An issue or pull request that needs to be reviewed labels Jul 18, 2023
@LuchoTurtle
Copy link
Member Author

This should work now 👌 . The issue was as expected, the env variables were not being loaded on the dev env.

image

@LuchoTurtle LuchoTurtle added the awaiting-review An issue or pull request that needs to be reviewed label Jul 18, 2023
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jul 18, 2023
@LuchoTurtle
Copy link
Member Author

@nelsonic before delving into this PR, please merge #101 first so dwyl/flutter-image-upload-demo#3 works. Thanks!

@nelsonic
Copy link
Member

Appears to be working on localhost: 🎉
image

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LuchoTurtle looks good. thanks! 🥇

@nelsonic nelsonic merged commit ce730d1 into main Jul 19, 2023
3 checks passed
@nelsonic nelsonic deleted the clientless-#62 branch July 19, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed documentation Improvements or additions to documentation elixir Pull requests that update Elixir code enhancement New feature or enhancement of existing functionality technical A technical issue that requires understanding of the code, infrastructure or dependencies user-feedback
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Feat: Filenames on S3 should be the cid of the content ...
2 participants