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 urls for root domains #1507

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Fix urls for root domains #1507

wants to merge 12 commits into from

Conversation

Pralish
Copy link
Contributor

@Pralish Pralish commented Mar 28, 2023

We are familiar with the fact that we need to pass host and/or subdomain parameters in url helper for it to work properly, especially for asset urls.

And we were passing either Subdomain.current.hostname or Apartment::Tenant.current as the subdomain, which is not consistent. Subdomain.current.hostname returns nil for root domain, which is fine, but Apartment::Tenant.current returns 'public' which causes all the asset urls to be prefixed with 'public'

rails_blob_url(attachment, subdomain: Apartment::Tenant.current, subdomain: Apartment::Tenant.current)

Issue: #1537

With this PR, we will no longer have to pass host and subdomain value to url helper. They are set by default based on the request host.

rails_blob_url(attachment)

You can pass the host and subdomain parameters if you want to override the default value

@donrestarone
Copy link
Contributor

donrestarone commented Mar 28, 2023

test plan 🧪

  1. API Renderer - car app, car video app ✅
  2. sign in ❌
  3. Password reset flow ❌
  4. account creation flow ⌛
  5. user invite flow ⌛
  6. forum file uploads ⌛
  7. CMS file uploads ⌛
  8. Forum post URL's ⌛
  9. temporary deployment to nikean.org ⌛
  10. downloading API Resources from an API Namespace (with non primitive properties) ⌛
  11. export Namespace in JSON format with associations (with non primitive properties) ⌛

@donrestarone
Copy link
Contributor

@Pralish I see the API surface for URL's and asset linking as changed, could you write some documentation to include as part of the release for server engineers to use as reference?

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@donrestarone donrestarone added the Pending UAT on Testnet ⚠️ currently on violet-test.net and being tested label Apr 24, 2023
@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1507.violet-test.net

@donrestarone
Copy link
Contributor

@Pralish could you please take a look at the review app? There are some major breakages that prevent UAT:

Admin redirect broken:

Screen.Recording.2023-05-02.at.7.37.24.AM.mov

password reset email link is broken

Screen Shot 2023-05-02 at 7 35 21 AM

@donrestarone donrestarone added Failing UAT ❌ ☠️ and removed Pending UAT on Testnet ⚠️ currently on violet-test.net and being tested labels May 2, 2023
@github-actions
Copy link

github-actions bot commented May 2, 2023

Deployed review-app can be viewed at https://review-1507.violet-test.net

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants