Avoid removing VMs from an archived/archiving plans. #1395
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1395 +/- ##
==========================================
- Coverage 36.81% 36.21% -0.60%
==========================================
Files 158 159 +1
Lines 2548 2579 +31
Branches 599 614 +15
==========================================
- Hits 938 934 -4
- Misses 1428 1630 +202
+ Partials 182 15 -167 ☔ View full report in Codecov by Sentry. |
|
lgtm |
| @@ -23,6 +24,15 @@ export const PlanVMsDeleteModal: React.FC<PlanVMsDeleteModalProps> = ({ plan, se | |||
| const vms = (plan?.spec?.vms || []).filter((vm) => !selected.includes(vm.id)) || []; | |||
|
|
|||
| React.useEffect(() => { | |||
| if (isPlanArchived(plan)) { | |||
| setAlertMessage( | |||
There was a problem hiding this comment.
why setting the whole comopnent as a state? make it much harder for React to compare, and not only the msg string? also, you will reduce the reputation in the code here of
<AlertMessageForModals title={t('Error')} message={something} />,
instead of declaring it 3 times, only once..
There was a problem hiding this comment.
in current line 100 u can do after it
{msg && <AlertMessageForModals title={t('Error')} message={msg} />}
| export const isPlanArchived = (plan: V1beta1Plan) => { | ||
| const planStatus = getPlanPhase({ obj: plan }); | ||
|
|
||
| return planStatus === 'Archiving' || planStatus === 'Archived'; |
There was a problem hiding this comment.
Archiving and Archived will probably be widely used, therefore it better to add them to a const file and prevent future typos
There was a problem hiding this comment.
Agree but current code is using the PlanPhase type based on strings and this type is used all over by comparing to strings.
Not complicated to refactor, but better do it in a follow up PR as well since touching other files.
There was a problem hiding this comment.
PlanPhase can be an enum and used in the compare planStatus === PlanPhase.Archiving etc... lets refactor it
There was a problem hiding this comment.
@metalice
Sure!
But I really prefer not to do it as part of this PR, but rather solve it today in a follow up one, since:
- it is not related to this fix and touches other components/pages so it's better to separate.
- It will generate unnecessary code conflicts since there is a merged code from me which is related to the plan status, e.g.
Let's merge the opened PRs relevant for plan status and I'll handle this issue separately.
There was a problem hiding this comment.
A follow up PR: #1408
sgratch
force-pushed
the
avoid-removing-vms-for-archived-plan
branch
from
a9e19b6 to
d338c44
Compare
|
Q: why do we show / enable the "remove virtual machine" button, in the case of archived/archiving plan ? |
Reference: https://issues.redhat.com/browse/MTV-1713 Avoid removing VMs from an archived/archiving plans. If a plan's status is either archiving or archived, block the option to remove VMs for that plan. Signed-off-by: Sharon Gratch <[email protected]>
sgratch
force-pushed
the
avoid-removing-vms-for-archived-plan
branch
from
d338c44 to
7c9a9f7
Compare
|
In all use cases (1 vm left, archived), we enable the button on the vms list page and once clicking, show the confirmation/error popup modal For an enhancement based on UX suggestion, I opened an issue: https://issues.redhat.com/browse/MTV-1727 |
|
/lgtm |
Reference: kubev2v#1395 (comment) Signed-off-by: Sharon Gratch <[email protected]>
Reference: kubev2v#1395 (comment) Signed-off-by: Sharon Gratch <[email protected]>
Reference: https://issues.redhat.com/browse/MTV-1713
Avoid removing VMs from an archived/archiving plans. If a plan's status is either archiving or archived, block the option to remove VMs for that plan.
Screenshots
Before
After