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 KVM checkbox when switching from other OS/Release to Ubuntu 18.04. #1048

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

Caleb-Ellis
Copy link
Contributor

Done

  • Fixed KVM checkbox being disabled when switching from any other OS/Release back to Ubuntu 18.04 if 18.04 is the first release in state.

QA

  • Make sure Ubuntu 18.04 is the earliest release available and you have at least 2 releases that aren't Ubuntu (bolla is set up this way).
  • Select deployable machines and open the deploy form
  • Change selection to Ubuntu 18.04 if not the default, and check that the KVM checkbox is enabled.
  • Change the OS and release then change back to Ubuntu
  • Check that the checkbox is enabled

Fixes

Fixes canonical-web-and-design/MAAS-squad#1904

// Whenever the selected OS changes, set the release value to the first OS's
// release in state.
const previousOS = usePrevious(values.oSystem);
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't quite figure out why this is in a useEffect and not on the field's onChange. Is there a reason? If so, maybe a comment would be useful here for future developers (probably me) wondering the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically because the list of releaseOptions is determined by the hook:

const releaseOptions = useSelector((state) =>
  generalSelectors.osInfo.getOsReleases(state, values.oSystem)
);

And since you can't have hooks in callbacks I've moved the logic up into the main component. I guess I could just have get all releaseOptions and then filter based on values.oSystem outside of the hook instead though.

@Caleb-Ellis Caleb-Ellis force-pushed the deploy-checkbox-fix branch from 3a2ef3d to b22ff4c Compare April 29, 2020 04:26
@Caleb-Ellis Caleb-Ellis merged commit 9775d4f into canonical:master Apr 29, 2020
@Caleb-Ellis Caleb-Ellis deleted the deploy-checkbox-fix branch April 29, 2020 07:01
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