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

Pass host to link_to_reportables helper #16060

Merged

Conversation

saraycp
Copy link
Contributor

@saraycp saraycp commented Apr 26, 2024

Fixes #16059

Screenshot 2024-04-26 at 13-56-42 Open Build Service

@saraycp saraycp added the review-app Apply this label if you want a review app started label Apr 26, 2024
@github-actions github-actions bot added the Frontend Things related to the OBS RoR app label Apr 26, 2024
@obs-bot
Copy link
Collaborator

obs-bot commented Apr 26, 2024

@saraycp saraycp marked this pull request as draft April 29, 2024 08:00
@saraycp saraycp force-pushed the fix_report_link_to_reported_element branch from 414dcce to 76023ae Compare April 29, 2024 08:41
@saraycp
Copy link
Contributor Author

saraycp commented Apr 29, 2024

Rebased on master

@saraycp saraycp marked this pull request as ready for review April 29, 2024 08:41
@saraycp saraycp force-pushed the fix_report_link_to_reported_element branch from 76023ae to e29a827 Compare April 29, 2024 08:45
Copy link
Contributor

@danidoni danidoni left a comment

Choose a reason for hiding this comment

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

I think we just need to use *_path helper methods and this will be ok... The report paths are not anything special, they do not need the host.

@hellcp-work
Copy link
Contributor

I think we just need to use *_path helper methods and this will be ok... The report paths are not anything special, they do not need the host.

This is also used in mailers, which is why full url is required in some cases

@danidoni
Copy link
Contributor

This is also used in mailers, which is why full url is required in some cases

Then we should not use this helper, it's leaking mailing-specific behavior into the reports view.

@hellcp-work
Copy link
Contributor

Then we should not use this helper, it's leaking mailing-specific behavior into the reports view.

This PR literally removes mailing specific behavior from this helper

@danidoni
Copy link
Contributor

This PR literally removes mailing specific behavior from this helper

Well, not really, the mailer specific behavior is still there, we just adapt the mailer so we can use it on regular pages. Anyway, looks like this helper is going away soon, so I won't block this anymore.

@danidoni danidoni dismissed their stale review April 29, 2024 10:28

I agree with the existing implementation

@saraycp saraycp force-pushed the fix_report_link_to_reported_element branch from e29a827 to 2b194b9 Compare April 29, 2024 10:36
@saraycp saraycp merged commit 820b577 into openSUSE:master Apr 29, 2024
20 of 21 checks passed
@saraycp saraycp deleted the fix_report_link_to_reported_element branch April 29, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app review-app Apply this label if you want a review app started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken link to reportable from report show page
4 participants