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 admin notification with relative URLs #4450

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Conversation

javierm
Copy link
Member

@javierm javierm commented Mar 29, 2021

Background

We had a test checking admin notifications worked with relative URLs. However, the test was passing because it was using the rack driver, but on real browsers it wasn't generating the expected URL.

Objectives

Fix wrong URL being generated on admin notifications with relative URLs.

@javierm javierm added the Bug label Mar 29, 2021
@javierm javierm self-assigned this Mar 29, 2021
@taitus taitus force-pushed the toggle_buttons branch 2 times, most recently from 00bc3c3 to 682eb2b Compare March 30, 2021 11:57
Copy link
Member

@Senen Senen left a comment

Choose a reason for hiding this comment

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

Nice catch! I was able to reproduce de error locally and check the commit fixes it! Although, it seems we have two flakes here.

We also have to rebase with the base branch as it changed recently.

@javierm javierm force-pushed the toggle_buttons branch 2 times, most recently from 322d6af to 4cf6794 Compare March 30, 2021 17:12
@Senen Senen self-assigned this Mar 30, 2021
@javierm javierm force-pushed the toggle_buttons branch 2 times, most recently from f422dcd to 6ea9383 Compare March 31, 2021 11:39
Base automatically changed from toggle_buttons to master March 31, 2021 11:52
The test was passing because it was using the rack driver, but on real
browsers it wasn't generating the expected URL.
@javierm javierm force-pushed the notification_relative_urls branch from eb59431 to d7563be Compare March 31, 2021 12:03
@javierm javierm merged commit 487f41c into master Mar 31, 2021
@javierm javierm deleted the notification_relative_urls branch March 31, 2021 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants