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

Fix image upload settings #135

Merged
merged 3 commits into from
Dec 19, 2023
Merged

Fix image upload settings #135

merged 3 commits into from
Dec 19, 2023

Conversation

jofmi
Copy link
Collaborator

@jofmi jofmi commented Nov 24, 2023

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Description

Fixed WriteJSON payload for updateSettings.

The wrong payload to WriteJSON broke the container without throwing an error - this issue is still not solved as I do not know the cause.

Checklist:

  • I have commented my code (or ChatGPT did), particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings, neither in my IDE nor in my browser
  • I have added tests that prove my fix is effective or that my feature works

@jofmi jofmi requested a review from lebe1 November 24, 2023 11:53
Copy link
Collaborator

@lebe1 lebe1 left a comment

Choose a reason for hiding this comment

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

Using your frontend branch fix-settings-update,I have two issues, I would like you to reproduce.

  1. Uploading an image with a long weird name like Screenshot from 2023-12-01 10-29-03.png leads to the error "GET http://localhost:3000/[object%20File] HTTP/1.1"
  2. Updating to the main item works fine with newspaper and calendar but item "Digitale Zeitung" leads to the error state that no database values are shown in the backoffice. I think this a frontend error of a hardcoded value, which I thought you solved @nanu-c but maybe not in the settingsupdate frontend page?

@jofmi
Copy link
Collaborator Author

jofmi commented Dec 1, 2023

Hi @lebe1, both errors do not appear in my local setup.

@lebe1
Copy link
Collaborator

lebe1 commented Dec 1, 2023

Using your frontend branch fix-settings-update,I have two issues, I would like you to reproduce.

  1. Uploading an image with a long weird name like Screenshot from 2023-12-01 10-29-03.png leads to the error "GET http://localhost:3000/[object%20File] HTTP/1.1"
  2. Updating to the main item works fine with newspaper and calendar but item "Digitale Zeitung" leads to the error state that no database values are shown in the backoffice. I think this a frontend error of a hardcoded value, which I thought you solved @nanu-c but maybe not in the settingsupdate frontend page?

After a more extended testing I found more details on these issues.

1. Error of uploading images
This error only occurs in my Chrome browser not in my Firefox browser. Therefore, I assume this is a Frontend error. The "GET http://localhost:3000/[object%20File] HTTP/1.1" call is created after selecting an image to be uploaded. @nanu-c do you have any ideas?

2. Error of changing main items several items
This error seems to me now as a backend error. In my Chrome browser as well as in my Firefox browser i can reproduce this error after several updating the main item between newspaper, calendar and digital license. Therefore, something still muss be broken in the backend, when not uploading an image during a settings update. @jofmi can you try reproducing this again?

@jofmi
Copy link
Collaborator Author

jofmi commented Dec 7, 2023

Hi @lebe1 I've been trying to change images and item back and forth and still cannot reproduce the error :/

@lebe1
Copy link
Collaborator

lebe1 commented Dec 8, 2023

Hi @jofmi and @nanu-c,
this issue is really giving me headaches. I would like to but really having a hard time simply ignoring this state of the system where the database is not connected to the backend and not throwing any errors.
In the following two videos I could reproduce both problems on Firefox:

Screencast_Firefox_DB_not_connected.webm
Screencast_Firefox_file_path.webm

Here are both errors shown on Chrome:

Screencast_Chrome_DB_not_connected.webm
Screencast_Chrome_file_path.webm

@nanu-c
Copy link
Collaborator

nanu-c commented Dec 13, 2023

1. Error of uploading images This error only occurs in my Chrome browser not in my Firefox browser. Therefore, I assume this is a Frontend error. The "GET http://localhost:3000/[object%20File] HTTP/1.1" call is created after selecting an image to be uploaded. @nanu-c do you have any ideas?

This part is now solved here -> augustin-wien/augustina-frontend#92

@nanu-c
Copy link
Collaborator

nanu-c commented Dec 13, 2023

And also i can't reproduce the main item change all breaks error either here.

nanu-c
nanu-c previously approved these changes Dec 13, 2023
log.Info("No file passed or file is invalid", err)
// Do not return error, as not passing a file is ok
// Could be improved by differentiating between not passed and invalid file
err = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe close the file, too here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not possible if there's an error, the file object cannot be closed since it has not been opened

@jofmi
Copy link
Collaborator Author

jofmi commented Dec 13, 2023

@lebe1 i have tried reproducing again with your video, with no success. i also tried to create a new item like it was already available in your video, which also created no issues. Can you see if the error still appears on your end if you use Aarons new branch augustin-wien/augustina-frontend#92? I also upgraded pgx to the latest version to see if this can fix your issue. Best i can suggest otherwise is that I take a look at the issue on your laptop directly.

@jofmi
Copy link
Collaborator Author

jofmi commented Dec 13, 2023

I have the thought that this might be caused by the frontend sending two overlapping db requests - since it happened when the frontend had the loop issue of sending GET and PATCH requests to the backend over and over again.

Maybe connected: jackc/pgx#635, esp. jackc/pgx#635 (comment)

@nanu-c, might there be a fundamental mistake how we do queries?

We could add some healthcheck config to pgxpool.New in db.go:120 https://pkg.go.dev/github.com/jackc/pgx/v5/pgxpool#ParseConfig

Could also help: https://github.com/uber-go/goleak

@jofmi
Copy link
Collaborator Author

jofmi commented Dec 13, 2023

I have added another improvement that closes all rows after a db query and added additional error logs.

@lebe1
Copy link
Collaborator

lebe1 commented Dec 13, 2023

@lebe1 i have tried reproducing again with your video, with no success. i also tried to create a new item like it was already available in your video, which also created no issues. Can you see if the error still appears on your end if you use Aarons new branch augustin-wien/augustina-frontend#92? I also upgraded pgx to the latest version to see if this can fix your issue. Best i can suggest otherwise is that I take a look at the issue on your laptop directly.

Just tested changing the Mainitem with the new frontend branch and I still ran into the same db-connection-loss error. I could also reproduce the same db-connection-loss error by uploading a .jpg file instead of a suggested .png file. Besides this uploading a new logo, which is a .png file works like a charme. Hopefully, this is just a local issue on my machine and changing the main item is really not a common use case but maybe we stick all our heads together this friday to decide how to go further with this?

@jofmi
Copy link
Collaborator Author

jofmi commented Dec 13, 2023

Still no errors in the logs with the latest version of this branch?

@nanu-c
Copy link
Collaborator

nanu-c commented Dec 13, 2023

deleted

@lebe1
Copy link
Collaborator

lebe1 commented Dec 18, 2023

Still no errors in the logs with the latest version of this branch?

Sadly no errors given besides the info No file passed or file is invalidhttp: no such file if I update the settings without an image. Still, it needs very few steps for me to run into this issue. Hopefully the production server does not as well since both of your machines did not have the problem... Therefore, I would give this one an approval to merge and create an issue to not forget about it.

@lebe1 lebe1 merged commit f877e62 into main Dec 19, 2023
2 checks passed
@lebe1 lebe1 deleted the fix-db branch December 19, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants