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

THREESCALE-8979: Fix the issue with not being able to remove default plan #3131

Merged

Conversation

mayorova
Copy link
Contributor

What this PR does / why we need it:

Fix the issue with not being able to remove default plan.

Which issue(s) this PR fixes

Fixes https://issues.redhat.com/browse/THREESCALE-8979

Verification steps

  1. Create a product, and an application plan for it
  2. Open the application plans list and see that No plan selected is shown for the Default plan
  3. in the drop-down select an application plan and click Change plan. The plan is changed and the "success" flash message is shown.
  4. Select No plan selected again, and click Change plan.
  5. The default plan is unset, the page is reloaded and the "success" flash message is shown.

Special notes for your reviewer:

In Flow there was an ignore message explaining why there was an empty string: https://github.com/3scale/porta/pull/2974/files#diff-5360738f38dabacb7a4d30fca57a8de39dd864cae351edae17bfe5c0b70fdae1R27

The ID of the default plan was changed from '' to -1 in the Typescript PR: https://github.com/3scale/porta/pull/3054/files

@mayorova mayorova changed the title Fix the issue with not being able to remove default plan THREESCALE-8979: Fix the issue with not being able to remove default plan Dec 12, 2022
@josemigallas
Copy link
Contributor

The whole id: number | string was a big mess (I guess it's still is) so let's better leave that alone. This should be handled by the Select, outside of it we don't care what the server needs. There is a leftover TODO from then migration to TS:

{/* TODO: should it be `value={item.id > -1 ? item.id : ''}`? */}

At the time I didn't remember why it was '' vs. -1 so I never answered the question.

Anyway, I think the biggest problem is there was no 🥒 test for this. That should come first.

@mayorova
Copy link
Contributor Author

let's better leave that alone
What do you mean @josemigallas ?

Do you mean that this fix is not valid, and should be done in a different way? (in the Select component)

@josemigallas
Copy link
Contributor

let's better leave that alone
What do you mean @josemigallas ?

Do you mean that this fix is not valid, and should be done in a different way? (in the Select component)

Yes

@mayorova mayorova force-pushed the THREESCALE-8979-fix-no-default-plan-selector branch from 8eac080 to 02c379f Compare December 20, 2022 13:39
@mayorova mayorova force-pushed the THREESCALE-8979-fix-no-default-plan-selector branch from 02c379f to f2b7cf0 Compare December 20, 2022 13:50
@@ -77,8 +77,8 @@ const Select = <T extends IRecord>({
>
{isLoading && <Spinner className="pf-u-ml-md" size="md" />}
{/* TODO: id should be treated as a string */}
{/* TODO: should it be `value={item.id > -1 ? item.id : ''}`? */}
{item && <input name={name} type="hidden" value={item.id} />}
{/* Controllers expect and empty string for some operations (such as unsetting the default plan) */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{/* Controllers expect and empty string for some operations (such as unsetting the default plan) */}
{/* Controllers expect an empty string for some operations (such as unsetting the default plan) */}

Copy link
Contributor

@josemigallas josemigallas left a comment

Choose a reason for hiding this comment

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

🥒

@mayorova mayorova merged commit 59722f1 into 3scale:master Dec 22, 2022
@mayorova mayorova deleted the THREESCALE-8979-fix-no-default-plan-selector branch December 22, 2022 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants