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(cc-domain-management): allow newly added domain to be marked as primary #1214

Conversation

florian-sanders-cc
Copy link
Contributor

@florian-sanders-cc florian-sanders-cc commented Oct 22, 2024

Fixes #1213

What does this PR do?

  • Fixes an issue when trying to set a domain that you have just associated to an app as primary.

How to review?

  • Check the commit,
  • Go to localhost:6006/demo-smart/, cc-domain-management, app-node:
  1. add a new domain (with no path prefix),
  2. mark it as primary,
  3. you should get no error,
  4. try to mark others as primary, delete some domains, nothing should throw any error.

@florian-sanders-cc florian-sanders-cc self-assigned this Oct 22, 2024
@florian-sanders-cc florian-sanders-cc force-pushed the cc-domain-management/fix-newly-added-domain-as-primary branch from 404575d to b75322f Compare October 22, 2024 14:37
Comment on lines -110 to -112
const hasPathPrefix = pathPrefix != null && pathPrefix !== '/';
// TODO: once the API returns actual ids, this should be adapted (either refetch or use the API response)
const id = hasPathPrefix ? hostname + pathPrefix : hostname;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot remember why I did that in the first place :/ I do remember that there were conflicts with some API routes not handling the trailing slash properly but it seems to work fine without it and usually I leave a comment to explain why I did something weird so it may be something I forgot to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the issue was as follows:

  • when adding a new domain, I used to not send trailing slashes to the API because that's what was done before,
  • so the id was the hostname without the path prefix if the path prefix was just "/",
  • but at some point I decided to send trailing slashes to the API when adding a domain and I forgot to update the id here to reflect that.

@florian-sanders-cc florian-sanders-cc force-pushed the cc-domain-management/fix-newly-added-domain-as-primary branch from b75322f to 68c3961 Compare October 22, 2024 14:41
Copy link
Contributor

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-domain-management/fix-newly-added-domain-as-primary/index.html.

This preview will be deleted once this PR is closed.

Copy link
Contributor

@HeleneAmouzou HeleneAmouzou left a comment

Choose a reason for hiding this comment

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

LGTM ! 👏

Copy link
Member

@hsablonniere hsablonniere left a comment

Choose a reason for hiding this comment

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

LGTM on the code, I haven't tested the demo.

@florian-sanders-cc florian-sanders-cc merged commit 9a51fb9 into master Oct 22, 2024
4 checks passed
@florian-sanders-cc florian-sanders-cc deleted the cc-domain-management/fix-newly-added-domain-as-primary branch October 22, 2024 16:43
Copy link
Contributor

🔎 The preview has been automatically deleted.

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.

cc-domain-management: impossible to set a newly added domain as primary
3 participants