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

#3764: Fix profile attachments saving #3782

Merged
merged 4 commits into from
Mar 3, 2024
Merged

#3764: Fix profile attachments saving #3782

merged 4 commits into from
Mar 3, 2024

Conversation

dorner
Copy link
Collaborator

@dorner dorner commented Jul 28, 2023

Tried this out and all the tests are still passing when I take the reload out, but attachments are saving again. Not sure why it was in there in the first place. Did some desultory manual testing as well and I couldn't find any problems.

@dorner dorner requested a review from cielf July 28, 2023 20:41
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @dorner. It might be a problem with my environment, but I am getting "This localhost page can't be found" when I try to open an uploaded file on my local.
Very light testing of counties looked ok (but would want to do more).

@dorner
Copy link
Collaborator Author

dorner commented Aug 4, 2023

@cielf not sure what happened but I tried again and it looks like I was missing one more reload - reloading inside a transaction looks like it blows away the attachment changes even if it's already changed. Try now!

@cielf
Copy link
Collaborator

cielf commented Aug 4, 2023

@dorner Hrm. On my local, If you upload a file, and have counties not adding up to 100, and hit save, you, of course, get an error (as you should), but there is a link for the file, and if you click on it, you get one of those unpleasant "This localhost page can’t be found" errors. I wouldn't be surprised to learn that was always the case, of course. We probably haven't tested these in combo much. (This was with the determination letter)

@dorner
Copy link
Collaborator Author

dorner commented Aug 11, 2023

@cielf played around and it looks like this only happens on the edit screen itself when there was an error. If you refresh the edit page, it shows you the original attachment (the new one wasn't saved, which is correct) and it's fine. Not sure it's worth it to chase this bug.

@cielf
Copy link
Collaborator

cielf commented Aug 16, 2023

So... if I understand correctly, we would be in a better place than we originally were if we put this in without "chasing this bug". Given that, I say let's have @awwaiid take a look, then move on.

@cielf
Copy link
Collaborator

cielf commented Feb 27, 2024

@awwaiid Can you take a look at this one, please?

Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

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

I agree -- this is better even if there is an error-on-edit-page-not-showing-attachment edge case.

@awwaiid awwaiid merged commit 4619735 into main Mar 3, 2024
12 checks passed
@awwaiid awwaiid deleted the 3764-fix-attachments branch March 3, 2024 15:58
Copy link
Contributor

@dorner: Your PR #3764: Fix profile attachments saving is part of today's Human Essentials production release: 2024.03.17.
Thank you very much for your contribution!

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