novu
novu copied to clipboard
🐛 Bug(s) and improvment(s) in settings page
Describe the bug
- Every selected logo-image gets uploaded to AWS-S3 bucket, even the ones which are not the final logo.
- On changing logo, the old logo isn't getting deleted from S3 bucket, even though new logo has been updated in settings page.
- 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:
- Open https://web.novu.co/settings page.
- Select a logo.
- Check Networks tab in browser's developer tool for the selected image's upload (PUT) request and GET request.
- Follow above steps for more than 1 image to check.
-
To check bug-2:
- Open https://web.novu.co/settings page.
- Open current logo's URL in new tab.
- Upload and update settings page with new logo and open new logo's URL in new tab.
- Refresh old logo's URL and it would still be available, indicating it isn't deleted from S3 bucket.
Expected behavior
- On updating logo, the old logo-image should get deleted from S3 bucket.
- The logo should get uploaded, only when
Update
button is pressed. - 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 👍
I would like to work on this issue 👍🏽
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?
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 🙏
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 🙌.
Sure think @devblin no worries. thanks for your contribution!
Hi @devblin , how's it going? need some help with this one?
@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 ?
- Trigger image update on S3, only when update button is clicked.
- Update existing key asset in AWS-S3 when updating image in settings-page.
- Image pre-processing (reducing size and dimension).
Hi @devblin , how's it going? need some help with this one?
I will let you know, in case I get stuck 👍🏽
Sure thing @devblin please let us know 🙏
Hi @devblin, Do you need any help in this?
@jainpawan21 Hey, thanks for asking, I am able to work on this, thanks again.
@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.
Hey @devblin! Mind to share the status of this issue? Have you seen the change requests @scopsy added to your PR?