Skip to content
This repository has been archived by the owner on Mar 25, 2023. It is now read-only.

feat(service-offering): show all service offerings even those which d… #1299

Merged
merged 13 commits into from
Oct 8, 2018

Conversation

HeyRoach
Copy link
Contributor

…on't fir account resource limitation

this.account = data.account;
}

public ngOnInit() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you use a component hook to do this instead if constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly we use ngOnInit for all the initialization/declaration and avoid stuff to work in the constructor. The constructor should only be used to initialize class members but shouldn't do actual "work".

memory: new FormControl(this.offering.memory, Validators.required),
cpuNumber: new FormControl(this.offering.cpunumber, [Validators.required, LimitValidator(this.maxCpu)]),
cpuSpeed: new FormControl(this.offering.cpuspeed, [Validators.required]),
memory: new FormControl(this.offering.memory, [Validators.required, LimitValidator(this.maxMemory)]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code does not work without this check?
The comment // input text=number provide all other validation for current restrictions describe that all validation provided by input itself. I think it should be placed back. You add only validation for max value, but input provide both max and min. See #1082

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To discuss

function LimitValidator(cpuNumberLimit: number): ValidatorFn {
return function (control: FormControl) {
return control.value > cpuNumberLimit
? { cpuLimitExceeded: true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the same as built-in validator Validators.max(max: number)

@@ -40,7 +42,7 @@ <h3 class="mat-dialog-title">
<button
mat-button
color="primary"
[disabled]="isSubmitButtonDisabled()"
[disabled]="isSubmitButtonDisabled() || resourcesLimitExceeded"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move to correspond function

[offeringList]="serviceOfferings"
[selectedOffering]="serviceOffering"
[showFields]="showFields"
[resourcesLimitExceeded]="resourcesLimitExceeded"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you decide to show this error in service-offering-list but not here?


if (offering.iscustomized) {
сpusExceeded = this.account.cpuavailable < this.serviceOffering['customOfferingRestrictions'].cpunumber.min;
memoryExceeded = this.account.memoryavailable < this.serviceOffering['customOfferingRestrictions'].memory.min;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you use square brackets to get customOfferingRestrictions field?


return enoughCpus && enoughMemory;
});
return availableInZone;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you do keep this function which only uses another function?

src/i18n/ru.json Outdated
@@ -118,6 +118,9 @@
},
"SNAPSHOT_POLICIES": {
"HOURLY_TURN_OFF": "Почасовое расписание отключено"
},
"COMPUTE_OFFERING": {
"RESOURCE_LIMIT_EXCEEDED": "Вычислительное предложение не может быть выбрано, потому что больше, чем доступные для аккаунта ресурсы."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we improve this translation? To discuss.

# Conflicts:
#	src/app/service-offering/service-offering-dialog/service-offering-dialog.component.ts
#	src/app/service-offering/service-offering-list/service-offering-list.component.ts
#	src/app/vm/selectors/service-offering.selectors.ts
#	src/app/vm/vm-creation/service-offering/vm-creation-service-offering.container.ts
@@ -9,3 +9,8 @@ h5 {
margin-top: 0;
}
}

.error-message {
color: red;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use color from mat-error

import { Component, Inject } from '@angular/core';
import { FormControl, FormGroup, Validators } from '@angular/forms'
import { Component, Inject, OnInit } from '@angular/core';
import { FormControl, FormGroup, ValidatorFn, Validators } from '@angular/forms'
Copy link
Collaborator

Choose a reason for hiding this comment

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

ValidatorFn unused

@@ -20,3 +20,8 @@
flex-direction: column;
text-align: right;
}

.error-message {
color: red;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Color

if (this.serviceOffering.iscustomized && this.viewMode === ServiceOfferingType.custom) {
return true;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Blank line

} from '../../../shared/models/config/custom-compute-offering-parameters.interface';

describe('ComputeOfferingViewModelSelector', () => {
describe('isAvailableByResources', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this describes?

// 'offeringId': '36de12ed-17f1-441f-903f-ab274832c318',
// 'cpunumber': {
// 'min': 2,
// 'max': 8,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented code

@@ -18,3 +18,8 @@
text-align: left;
padding-top: 18px;
}

.error-message {
color: red;
Copy link
Collaborator

Choose a reason for hiding this comment

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

color

@@ -2,7 +2,7 @@ import { Component, OnInit } from '@angular/core';
import { MatDialogRef } from '@angular/material';
import { select, Store } from '@ngrx/store';
import { combineLatest, Observable } from 'rxjs';
import { first, map } from 'rxjs/operators';
import { first, filter, map } from 'rxjs/operators';
Copy link
Collaborator

Choose a reason for hiding this comment

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

filter unused

@@ -1 +1 @@
export { getComputeOfferingViewModel } from './compute-offering-view-model.selector';
export { getComputeOfferingForVmCreation } from './compute-offering-view-model.selector';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you did not import the second selector?

customComputeOfferingParameters,
defaultRestrictions,
defaultHardwareValues,
tags, vm): ComputeOfferingViewModel[] => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put each parameter on new line, or all parameters in one line

: Number(account.memoryavailable) + memoryUsed;

const availableResources: Resources = {
cpuNumber: cpuNumber || cpuNumberUsed,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment about fallback value

defaultHardwareValues,
tags): ComputeOfferingViewModel[] => {
const availableResources: Resources = {
cpuNumber: account && account.cpuavailable || '0',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment about fallback value

# Conflicts:
#	src/app/shared/models/account.model.ts
#	src/app/vm/vm-creation/vm-creation.component.html
#	src/app/vm/vm-creation/vm-creation.component.ts
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1078

  • 83 of 106 (78.3%) changed or added relevant lines in 15 files are covered.
  • 156 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.2%) to 57.846%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/app/vm/vm-creation/service-offering/vm-creation-service-offering.container.ts 2 3 66.67%
src/app/vm/vm-creation/vm-creation.component.ts 3 4 75.0%
src/app/service-offering/custom-service-offering/custom-service-offering.component.ts 7 9 77.78%
src/app/service-offering/service-offering-list/service-offering-list.component.ts 2 5 40.0%
src/app/vm/selectors/service-offering.selectors.ts 3 6 50.0%
src/app/reducers/vm/redux/vm-creation.effects.ts 1 7 14.29%
src/app/service-offering/service-offering-dialog/service-offering-dialog.component.ts 1 8 12.5%
Files with Coverage Reduction New Missed Lines %
src/app/vm/selectors/view-models/compute-offering-view-model.selector.ts 3 86.71%
src/app/service-offering/custom-service-offering/custom-service-offering.component.ts 7 55.56%
src/app/service-offering/service-offering-dialog/service-offering-dialog.component.ts 7 37.97%
src/app/vm/vm-creation/service-offering/vm-creation-service-offering.container.ts 8 54.02%
src/app/vm/vm-creation/vm-creation.component.ts 11 46.24%
src/app/vm/selectors/service-offering.selectors.ts 13 55.28%
src/app/service-offering/service-offering-list/service-offering-list.component.ts 15 42.11%
src/app/reducers/vm/redux/vm-creation.effects.ts 92 23.96%
Totals Coverage Status
Change from base Build 1074: 0.2%
Covered Lines: 12189
Relevant Lines: 18337

💛 - Coveralls

@HeyRoach HeyRoach merged commit 8eb44a4 into bwsw:master Oct 8, 2018
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.

3 participants