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 contact insertion #3305

Merged
merged 3 commits into from
Jan 11, 2025
Merged

Fix contact insertion #3305

merged 3 commits into from
Jan 11, 2025

Conversation

david-venhoff
Copy link
Member

@david-venhoff david-venhoff commented Dec 19, 2024

Short description

On the testserver we have multiple possible backend urls, and the contacts were previously loaded using their complete server url. For example, if a user was on https://cms-test.integreat-app.de the contact was still loaded using https://integreat-test.tuerantuer.org. Then the login form would be returned, because the user had of course no valid session for the other url.

So instead of using full urls, we can just use relative urls, like we do for all other links.

It's a bit hard to test this pr, because everything worked anyways on localhost, but I hope the explanation helps :)

I also considered adding redirect: "error", to the fetch call. This would at least have prevented that the full login form gets pasted. But maybe that is something we should add to all our fetch calls?

Proposed changes

  • Don't use the full url

Side effects

Should be none

Resolved issues

Fixes: #3304


Pull Request Review Guidelines

On the testserver, we have multiple possible backend urls.
So instead of using full urls, we can just use relative urls
(For some reason called absolute urls in our codebase)
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on so quickly!

Curious, do you think we could just adjust the full_url definition?

@david-venhoff
Copy link
Member Author

david-venhoff commented Dec 19, 2024

Curious, do you think we could just adjust the full_url definition?

I think that should also be possible. It would probably be a bit more robust, because it would make contact cards independent of base url changes

@david-venhoff
Copy link
Member Author

@charludo
I tried your approach and one problem is that apparently tinymce tries to make every url absolute. So for example if the contact cards contains the url /augsburg/contact/5 then tinymce will convert it into https://integreat.app/augsburg/contact/5/ and that will mean that the get_referencing_x functions will not find it anymore, because they only look for relative urls…

I'm not sure if there is a way to get this to work, do you have an idea?

@charludo
Copy link
Contributor

charludo commented Dec 21, 2024

@charludo I tried your approach and one problem is that apparently tinymce tries to make every url absolute. So for example if the contact cards contains the url /augsburg/contact/5 then tinymce will convert it into https://integreat.app/augsburg/contact/5/ and that will mean that the get_referencing_x functions will not find it anymore, because they only look for relative urls…

I'm not sure if there is a way to get this to work, do you have an idea?

Huh, I didn't even think to make the URLs relative - I think that may be a great solution though? Although that might mean we need to expose the contacts via API as well

Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

I am sorry, I just re-read my comment from before the holidays, and it makes no sense. I have no idea what I was thinking. Super sorry about that.

Upon reconsideration, this looks like a great solution to the problem 😄 🎉

@charludo
Copy link
Contributor

charludo commented Jan 8, 2025

Curious, do you think we could just adjust the full_url definition?

I think that should also be possible. It would probably be a bit more robust, because it would make contact cards independent of base url changes

Actually, could you implement this solution after all? 🙈
I'm working on #3258 and it's become necessary to use the full_url in additional places, so I think we should solve this issue at the lowest level possible.

Sorry for the flip-flopping 😭

@charludo
Copy link
Contributor

@david-venhoff I made the changes we talked about, would you might taking another look if this is OK like this?

@charludo charludo force-pushed the bugfix/contact-insertion branch 2 times, most recently from 4b482f7 to e17bb40 Compare January 10, 2025 06:51
Copy link
Member Author

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Looks good to me. Lets hope that it works

integreat_cms/static/src/js/forms/tinymce-init.ts Outdated Show resolved Hide resolved
@charludo charludo force-pushed the bugfix/contact-insertion branch from e17bb40 to 901fc45 Compare January 11, 2025 08:03
@charludo charludo added effort: low Should be doable in <4h deadline Needs to be fixed in the given time blocker This issue blocks another issue and removed effort: low Should be doable in <4h deadline Needs to be fixed in the given time blocker This issue blocks another issue labels Jan 11, 2025
@charludo charludo merged commit 5ab3a33 into develop Jan 11, 2025
5 checks passed
@charludo charludo deleted the bugfix/contact-insertion branch January 11, 2025 08:16
charludo added a commit that referenced this pull request Jan 13, 2025
* Fix contact insertion

On the testserver, we have multiple possible backend urls.
So instead of using full urls, we can just use relative urls
(For some reason called absolute urls in our codebase)

* Just use `reverse` instead of manually coding url

* Only save the absolute path

---------

Co-authored-by: charludo <github@charlotteharludo.com>
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.

Insert contact broken on test server
3 participants