novu icon indicating copy to clipboard operation
novu copied to clipboard

🐛 Bug(s) and improvment(s) in settings page

Open devblin opened this issue 2 years ago • 13 comments

Describe the bug

  1. Every selected logo-image gets uploaded to AWS-S3 bucket, even the ones which are not the final logo.
  2. On changing logo, the old logo isn't getting deleted from S3 bucket, even though new logo has been updated in settings page.
  3. Currently, the logo-image selected by user isn't processed in the client-side. If logo-image size is large (both in size and dimension), it should be processed to keep the image within a threshold size and dimension as required in settings page.

To Reproduce Steps to reproduce the behavior:

  • To check bug-1:

    1. Open https://web.novu.co/settings page.
    2. Select a logo.
    3. Check Networks tab in browser's developer tool for the selected image's upload (PUT) request and GET request.
    4. Follow above steps for more than 1 image to check.
  • To check bug-2:

    1. Open https://web.novu.co/settings page.
    2. Open current logo's URL in new tab.
    3. Upload and update settings page with new logo and open new logo's URL in new tab.
    4. Refresh old logo's URL and it would still be available, indicating it isn't deleted from S3 bucket.

Expected behavior

  1. On updating logo, the old logo-image should get deleted from S3 bucket.
  2. The logo should get uploaded, only when Update button is pressed.
  3. The logo-image selected by user should be properly processed (both in size and dimension) in order to avoid unwanted memory usage by image in S3 bucket.

Details (please complete the following information):

  • OS: Linux
  • Node Version: v16.14.0

Additional context

  • I have tested and verified above bugs in local development environment.

I am willing to submit a PR 👍

devblin avatar Jul 12 '22 18:07 devblin

I would like to work on this issue 👍🏽

devblin avatar Jul 12 '22 18:07 devblin

Hi @devblin all are great suggestions except the removal of the old logo. This might break some email client who don't cache the image.

We can replace the asset using the same key on S3, which might be ok in this case. But to be honest, I don't belive that pricing on S3 is a big concern when it comes to hosting those image types.

Limiting the client and also on the backend for the actual image upload is a great idea. Maybe when generating the s3 upload link we can add a limit on amazons side?

scopsy avatar Jul 12 '22 20:07 scopsy

Hi @devblin just wanted to check-in to see if you will need any help here :) I'm available for any questions you might have 🙏

scopsy avatar Aug 01 '22 17:08 scopsy

Hey @scopsy, sorry for being inactive, was caught up due to some academic stuffs. I will share my approach and realted queries by tommorow and hope to fix this issue within this week 🙌.

devblin avatar Aug 01 '22 17:08 devblin

Sure think @devblin no worries. thanks for your contribution!

scopsy avatar Aug 12 '22 09:08 scopsy

Hi @devblin , how's it going? need some help with this one?

oba2311 avatar Aug 29 '22 15:08 oba2311

@scopsy @oba2311 Sorry for the delay, so, I will be proceeding with the implementation and creating 3 PRs for each of the following fix/refactor mentioned below along with the approaches. Currently, working on 1st, will make the PR soon. Let me know if, should I work on below mentioned 3rd refactor too ?

  1. Trigger image update on S3, only when update button is clicked.
  2. Update existing key asset in AWS-S3 when updating image in settings-page.
  3. Image pre-processing (reducing size and dimension).

devblin avatar Aug 29 '22 16:08 devblin

Hi @devblin , how's it going? need some help with this one?

I will let you know, in case I get stuck 👍🏽

devblin avatar Aug 29 '22 16:08 devblin

Sure thing @devblin please let us know 🙏

scopsy avatar Sep 04 '22 15:09 scopsy

Hi @devblin, Do you need any help in this?

jainpawan21 avatar Oct 07 '22 16:10 jainpawan21

@jainpawan21 Hey, thanks for asking, I am able to work on this, thanks again.

devblin avatar Oct 07 '22 18:10 devblin

@scopsy Please review PR: https://github.com/novuhq/novu/pull/1553/files , this fixes the first bug mentioned. For other bug fixes, I will make the PR by today.

devblin avatar Oct 08 '22 06:10 devblin

Hey @devblin! Mind to share the status of this issue? Have you seen the change requests @scopsy added to your PR?

iampearceman avatar Dec 01 '22 01:12 iampearceman