8000 fix: redirect to new url after template name update by Kira-Pilot · Pull Request #10926 · coder/coder · GitHub
[go: up one dir, main page]

Skip to content

fix: redirect to new url after template name update #10926

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

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

Kira-Pilot
Copy link
Member

resolves #10634

@Kira-Pilot Kira-Pilot requested review from a team and Parkreiner and removed request for a team November 28, 2023 19:05
Copy link
Member
@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Looks really good!
All the code was straightforward and easy to follow, but I did notice one edge case with the test helper that could cause flakes/weird behavior if we're not careful

});
await expect(page).toHaveURL(`/templates/${templateName}/settings`);

for (const [key, value] of Object.entries(templateSettingValues)) {
Copy link
Member
@Parkreiner Parkreiner Nov 28, 2023

Choose a reason for hiding this comment

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

There's nothing wrong with the code right now, but there is a risk in the future from runtime/compile-time mismatches:

const myObj = {
  name: "whoops",
  obviouslyWrongValue: true
};

// No error
updateTemplateSettings(page, templateName, myObj);

Just to be on the safe side, and because this is a general test helper, I'm wondering if there should be a runtime check on the key to make sure that you're only ever processing name/display_name/description

Copy link
Member

Choose a reason for hiding this comment

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

Edited the message because I didn't like the way my tone sounded. Sorry about that

Copy link
Member
@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Kira and I talked about the issue – changes were kind of overkill, since we will still get errors if the runtime looping causes problems

Switching this to approved

@Kira-Pilot Kira-Pilot merged commit d374bec into main Nov 29, 2023
@Kira-Pilot Kira-Pilot deleted the template-name-change/kira-pilot branch November 29, 2023 15:54
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: renaming a template does not refresh the UI with the new name, causing additional edits to fail
2 participants
0