Bug 11405505:[UR2] Bring Environment Switcher and QuotaSettings API to Functions Portal to support the new Websites blades based on IFrame like AppSettings #2593

exc3eed · 2018-04-20T08:43:05Z

Bug 11405505:[UR2] Bring Environment Switcher and QuotaSettings API to Functions Portal to support the new Websites blades based on IFrame like AppSettings

…to Functions Portal to support the new Websites blades based on IFrame like AppSettings

exc3eed · 2018-04-20T08:46:38Z

@joaquinv is added to the review. #Closed

…to Functions Portal to support the new Websites blades based on IFrame like AppSettings

nertim · 2018-04-20T18:13:49Z

client/src/app/shared/models/arm/quotaSettings.ts

+    Shared = 0,
+    Dedicated = 1
+}
+


no need to have =0, =1

nertim · 2018-04-20T18:14:32Z

client/src/app/shared/models/arm/quotaSettings.ts

+        quotaSettings: Array<{
+            key: string,
+            value: string
+        }>


why not use { [key: string]: string } for quotaSettings type?

quotaSettings field is an array of object havihg structure like {key: string, value: string} I can't understand how [ { [key: string]: string } fit in this case. Can you explain more

In reply to: 183130987 [](ancestors = 183130987)

well in this case, can the key be repeated? if not, than it the same thing as a dictionary.. which can represented via the '{ [key: string]: string }' type.

Here is an example response from API. We need to represent object property value. in '{ [key: string]: string } how are we represing this (value) in dictionries?
rties
:
{subscriptionId: "023e06f0-5e20-430f-baae-649afc496c41",…}
adminId
:
"antaresadmin@onappservice.onmicrosoft.com"
coAdminIds
:
null
quotaSettings
:
[{key: "BackupPeriodHours_Dynamic_WebSpace_Dynamic",…},…]
[0 … 99]
0
:
{key: "BackupPeriodHours_Dynamic_WebSpace_Dynamic",…}
key
:
"BackupPeriodHours_Dynamic_WebSpace_Dynamic"
value
:
"{"ComputeMode":2,"CustomActionName":null,"EnforcementScope":0,"ExceededAction":0,"IncludedAmount":null,"IsRolloverEnabled":false,"Limit":-1,"Period":null,"QuotaName":"BackupPeriodHours","QuotaType":0,"ResourceName":"BackupPeriodHours","RolloverResetPeriod":null,"RolloverResetUnit":null,"SKU":"Dynamic","SiteMode":null,"Unit":null,"WebPlan":"DefaultSkuPlan"}"
1
:
{key: "BytesReceived_Dynamic_WebSpace_Dynamic",…}
key
:
"BytesReceived_Dynamic_WebSpace_Dynamic"
value
:
"{"ComputeMode":2,"CustomActionName":null,"EnforcementScope":0,"ExceededAction":0,"IncludedAmount":null,"IsRolloverEnabled":false,"Limit":-1,"Period":1440,"QuotaName":"BytesReceived","QuotaType":0,"ResourceName":"BytesReceived","RolloverResetPeriod":1,"RolloverResetUnit":3,"SKU":"Dynamic","SiteMode":null,"Unit":1,"WebPlan":"DefaultSkuPlan"}"
2
:
{key: "BytesReceived_Dynamic_Site_Dynamic",…}
key
:
"BytesReceived_Dynamic_Site_Dynamic"
value
:
"{"ComputeMode":2,"CustomActionName":null,"EnforcementScope":1,"ExceededAction":0,"IncludedAmount":null,"IsRolloverEnabled":false,"Limit":-1,"Period":1440,"QuotaName":"BytesReceived","QuotaType":0,"ResourceName":"BytesReceived","RolloverResetPeriod":1,"RolloverResetUnit":3,"SKU":"Dynamic","SiteMode":null,"Unit":1,"WebPlan":"DefaultSkuPlan"}"
3
:
{key: "BytesSent_Dynamic_WebSpace_Dynamic",…}

In reply to: 183445695 [](ancestors = 183445695)

Seems like there's something wrong with your API if it's returning items with the exact same key. I agree with Mitren that a dictionary makes more sense than a list in this case.

In reply to: 183499724 [](ancestors = 183499724,183445695)

Besides, you're just doing a "find" on the first result that matches your key anyway, so you're basically treating it like a dictionary which isn't deterministic if your API is returning duplicate keys.

In reply to: 183503238 [](ancestors = 183503238,183499724,183445695)

Well API doesn't return dictionary. But we can create dictionary from the object. but fixing API is out of scope of this CR.
I think the confusion came from the property name key and value

[
{
Key: "k1"
value: "v1"
},
{
Key: "k2"
value: "v2"
},
{
Key: "k3"
value: "v3"
},
{
Key: "k4"
value: "v4"
},
]

Can above array be represented as { [key: string]: string } where is the value field?

In reply to: 183504777 [](ancestors = 183504777,183503238,183499724,183445695)

so ' { [key: string]: string }' is a representation of dictionary. e.g.
let x: { [key: string]: string };
x['k1'] = 'v1';
x['k2'] = 'v2';

Right. But our response is not a dictionary. So we cannot use this interface to represent the API response. Correct me if I am wrong.

In reply to: 183520024 [](ancestors = 183520024)

ah. okay. In that case lets leave the current implementation.

nertim · 2018-04-20T18:15:19Z

client/src/app/shared/models/arm/site.ts

@@ -20,4 +20,5 @@ export interface Site {
    hostingEnvironmentProfile?: HostingEnvironmentProfile;
    name?: string;
    resourceGroup?: string;
+    computeMode?: number;


should be of type ComputeMode?

in api response it is computeMode. so we have to keep it like this way

In reply to: 183131211 [](ancestors = 183131211)

Yeah but I'm sure that number maps to an enum. I think it maps to ComputeModeOptions in the backend.

In reply to: 183193984 [](ancestors = 183193984,183131211)

@exc3eed I think what he means is this could be an enum which would read a lot easier.

There are several other values in that object that seem to be enums, but returned as numbers. Can you please check to see what they are represented as in our existing codebase?

Yes we should review others field as well
But for now lets just focus on current scope computemode as we need to finish this ASAP.

In reply to: 183520796 [](ancestors = 183520796)

What I mean is that you have a compute mode enum defined. but in this code you have it as a number. so there two things you can do...

is either update the object based on the response to be an enum, so 'computeMode?: ComputeMode'. Now this is a little risky if the enum changes order. OR

Instead of having an enum defined, just have constants defined of computeMode and their corresponding number.

What I find weird here is that you've got an enum that you're comparing the number with. To reduce confusion for anyone looking at the code in the future, it would be a good idea to stick with one type across all usecases.

Also what happens if you receive an incorrect number? in your case, what if computeMode of 4 comes in the response.

nertim · 2018-04-20T18:16:55Z

client/src/app/shared/services/quota.service.ts

+        const resourceId = this._quotaSettingsResourceIdFormat.format(subscriptionId);
+        return  this._cacheService.getArm(resourceId).map(r => {
+            const quotaSettings: QuotaSettings[] = r.json().value;
+            const quotaSetting = quotaSettings.find(quota => quota.properties.subscriptionId === subscriptionId);


isn't call getting the quota settings for a given subscription?

yes but quota settings api return quotasettings for all subscription under the current logged in user.
This method supposed to return
quota settings of the current subscription. That's why filtering

In reply to: 183131600 [](ancestors = 183131600)

my apologies for harping on this further.. but I am a little confused.
The ARM call is taking in a quotaSettingsResourceId which contains the subscriptionId. Are you saying this call is returning quotaSettings for subscriptions other than the subscriptionId in the resourceId?

Yes. it is little confusing. the API returns all quota settings of all subscriptions under current user.

In reply to: 183521936 [](ancestors = 183521936)

So in this case, if issue is with API than not much can be done here... But can you please open a bug on the API to only return for the given sub? and add a todo here with the bug number?

My worry is that a user could possibly have 100s of subs so this can get pretty hairy.

In reply to: 183543195 [](ancestors = 183543195,183521936)

nertim · 2018-04-20T18:19:04Z

client/src/app/shared/services/quota.service.ts

+        private _cacheService: CacheService) {
+    }
+
+    getQuotaSettings(subscriptionId: string):  Observable<QuotaSettings> {


nit... is this returning a QuotaSetting or QuotaSettings? update method name to reflect.

quota settings api return quotasettings for all subscription under the current logged in user.
This method supposed to return
quota settings of the current subscription. I think the confusion come from singular return. So method name here is correct but return value name is confusing. will fix it

In reply to: 183132157 [](ancestors = 183132157)

if issue is with API than not much can be done here... But can you please open a bug on the API to only return for the given sub? and add a todo here with the bug number?

My worry is that a user could possibly have 100s of subs so this can get pretty hairy.

nertim · 2018-04-20T18:20:18Z

client/src/app/shared/services/quota.service.ts

+            // since quota setting doesn’t change that often.
+            // Also object lookup is faster than array iteration.
+            return this.getQuotaSettings(subscriptionId).map(quotaSetting => {
+                const quota = quotaSetting.properties.quotaSettings.find(qs => qs.key.toLowerCase() === fullQuotaName);


nit. reading this code is a little confusing.. quotasetting.properties.quotasettings.. maybe name quotaSetting to something else?

nertim · 2018-04-20T18:20:50Z

client/src/app/shared/services/quota.service.ts

+            // since quota setting doesn’t change that often.
+            // Also object lookup is faster than array iteration.
+            return this.getQuotaSettings(subscriptionId).map(quotaSetting => {
+                const quota = quotaSetting.properties.quotaSettings.find(qs => qs.key.toLowerCase() === fullQuotaName);


Also see if you can use { [key: string]: string } for the quota settings.

nertim · 2018-04-20T18:21:52Z

client/src/app/shared/services/quota.service.ts

+        quotaName: string,
+        sku: string,
+        computeMode: ComputeMode,
+        quotaScope: QuotaScope = QuotaScope.WebSpace): string {


make generateFullQuotaName private? also at this point quotaScope should be passed in rather than having a default.

nertim

Some small questions/fixes.

jvano · 2018-04-20T20:24:38Z

@ehamai is added to the review. #Closed

ehamai · 2018-04-23T14:54:33Z

client/src/app/shared/services/quota.service.ts

+
+    getQuotaSettings(subscriptionId: string):  Observable<QuotaSettings> {
+        const resourceId = this._quotaSettingsResourceIdFormat.format(subscriptionId);
+        return  this._cacheService.getArm(resourceId).map(r => {


this._cacheService.getArm [](start = 15, length = 26)

Make sure you're using the ConditionalHttpClient to make your requests. See the SiteService for an example. The difference is that the client will make sure that failing requests never fail the observable, but instead return an object with isSucceeded as true/false. This forces you to handle error cases, otherwise failed observables can have really bad consequences to the UI.

ehamai · 2018-04-23T15:00:37Z

client/src/app/shared/services/quota.service.ts

+            });
+    }
+
+    private generateFullQuotaName(


generateFullQuotaName [](start = 12, length = 21)

_generateFullQuotaName

ehamai · 2018-04-23T15:01:52Z

client/src/app/shared/services/scenario/onprem.environment.ts

+import { Injector } from '@angular/core';
+
+
+

 export class OnPremEnvironment extends Environment {


nit: remove all the empty lines

ehamai · 2018-04-23T15:06:20Z

client/src/app/shared/services/scenario/onprem.environment.ts

+                    input.site.properties.computeMode
+                ).map(limit => {
+                    return <ScenarioResult> {
+                        status: limit !== 0 ? 'enabled' : 'disabled',


!== 0 [](start = 38, length = 5)

!== 0 seems a little weird for enable/disable. So if limit were -1, then that's enabled? Maybe > 0 makes more sense? Or if it's truly only 0 or 1 are possible values, then actually check for it:

limit === 1 ? 'enabled' : 'disabled'

-1 means infinite capacity. so we can not use limit > 0
According to current design only limit === 0 means it is disabled

In reply to: 183429486 [](ancestors = 183429486)

ehamai · 2018-04-23T15:06:46Z

client/src/app/shared/services/scenario/onprem.environment.ts

+                    input.site.properties.computeMode
+                ).map(limit => {
+                    return <ScenarioResult> {
+                        status: limit !== 0 ? 'enabled' : 'disabled',


limit !== 0 [](start = 32, length = 12)

Same here.

hartra344 · 2018-04-23T15:07:47Z

client/src/app/site/site-config/general-settings/general-settings.component.html

@@ -126,7 +126,10 @@ <h3 class="first-config-heading">{{ 'feature_generalSettingsName' | translate }}

        <span check-scenario="EnablePlatform64" [cs-input]="{site: siteArm}" cs-enabledClass="hidden" cs-disabledClass="">
          <a tabindex="0" (click)="scaleUp()">{{'upgradeToEnable' | translate}}</a>
-          <pop-over [message]="'use32BitWorkerProcessUpsell' | translate">
+          <pop-over *ngIf="!useGenericUpsellMessage" [message]="'use32BitWorkerProcessUpsell' | translate">


instead of using ngIf here can we just set and pass in the correct message from code? This method is more expensive per wise and it's not as clean

Ditto

In reply to: 183429967 [](ancestors = 183429967)

ehamai · 2018-04-23T15:07:57Z

client/src/app/shared/services/scenario/scenario.service.ts

@@ -23,7 +23,6 @@ export class ScenarioService {

    private _environments: Environment[] = [
        new StandaloneEnvironment(),
-        new OnPremEnvironment(),


new OnPremEnvironment(), [](start = 8, length = 24)

Leave this here. Just make injector a private variable in the Scenario service and pass it into the constructor.

hartra344 · 2018-04-23T15:09:12Z

client/src/app/site/site-config/general-settings/general-settings.component.html

@@ -165,7 +173,10 @@ <h3 class="first-config-heading">{{ 'feature_generalSettingsName' | translate }}

        <span check-scenario="EnableAlwaysOn" [cs-input]="{site: siteArm}" cs-enabledClass="hidden" cs-disabledClass="">
          <a tabindex="0" (click)="scaleUp()">{{'upgradeToEnable' | translate}}</a>
-          <pop-over [message]="'alwaysOnUpsell' | translate">
+          <pop-over *ngIf="!useGenericUpsellMessage" [message]="'alwaysOnUpsell' | translate">


again better to set this message in code i think.

hartra344 · 2018-04-23T15:15:26Z

client/src/app/site/site-config/general-settings/general-settings.component.ts

@@ -100,6 +103,7 @@ export class GeneralSettingsComponent extends ConfigSaveComponent implements OnC
        this._resetPermissionsAndLoadingState();

        this._generateRadioOptions();
+        this.useGenericUpsellMessage = this._configService.isOnPrem();


yeah here, instead of saying 'useGenericUpsellMessage' as a boolean, you should just set the message content

ehamai · 2018-04-23T15:17:31Z

client/src/app/site/site-config/general-settings/general-settings.component.html

        </radio-selector>
+        <span check-scenario="WebSocketsEnabled" [cs-input]="{site: siteArm}" cs-enabledClass="hidden" cs-disabledClass="">
+          <a tabindex="0" (click)="scaleUp()">{{'upgradeToEnable' | translate}}</a>


<a tabindex="0" (click)="scaleUp()">{{'upgradeToEnable' | translate}} [](start = 10, length = 73)

There should be an upsell.svg icon after the link as well to match previous upsell messages.

ehamai · 2018-04-23T15:20:03Z

server/Resources/Resources.resx

@@ -1468,6 +1468,9 @@
  <data name="alwaysOnUpsell" xml:space="preserve">
    <value>Always on requires a basic or higher App Service plan</value>
  </data>
+  <data name="upgradeUpsell" xml:space="preserve">
+    <value>Current plan does not support this feature. Please contact Administrator to enable this or upgrade your plan to enable</value>


Current plan does not support this feature. Please contact Administrator to enable this or upgrade your plan to enable [](start = 11, length = 118)

The App Service plan associated with this app does not support support this feature. Please contact your administrator to enable this feature for your current plan, or upgrade to a plan that already as it enabled.

ehamai

🕐

nertim

Had a quick chat with @exc3eed ... my two major concerns are:

consistency... try to be consistent with type and nullables.
File a bug on the API layer to honor the subscriptionId and add a todo in the code with the bug. My worry is that if we end up with a user with too many subscriptions, we might end up having issues.

…e-functions-ux into asfarok-envswitcher

…o asfarok-envswitcher

nertim · 2018-04-25T16:07:05Z

client/src/app/shared/services/quota.service.ts

+
+@Injectable()
+export class QuotaService {
+    private readonly _site = 'site';


private static readonly

nertim

Thank you for your patience :) please also make sure rest of the teams' comments are also addressed.

ehamai

hartra344 · 2018-05-16T16:46:03Z

@exc3eed Is this PR still needed or is it rolled into #2699 ?

BUG: 11405505:[UR2] Bring Environment Switcher and QuotaSettings API …

7cd1558

…to Functions Portal to support the new Websites blades based on IFrame like AppSettings

BUG: 11405505:[UR2] Bring Environment Switcher and QuotaSettings API …

e9dbd6f

…to Functions Portal to support the new Websites blades based on IFrame like AppSettings

Azure deleted a comment Apr 20, 2018

nertim reviewed Apr 20, 2018

View reviewed changes

nertim suggested changes Apr 20, 2018

View reviewed changes

exc3eed added 2 commits April 20, 2018 19:29

added different upsell message for onprem

27ab7a4

Merge branch 'dev' into asfarok-envswitcher

c1da00d

ehamai reviewed Apr 23, 2018

View reviewed changes

hartra344 reviewed Apr 23, 2018

View reviewed changes

ehamai reviewed Apr 23, 2018

View reviewed changes

hartra344 reviewed Apr 23, 2018

View reviewed changes

hartra344 requested changes Apr 23, 2018

View reviewed changes

ehamai reviewed Apr 23, 2018

View reviewed changes

ehamai requested changes Apr 23, 2018

View reviewed changes

Azure deleted a comment from exc3eed Apr 23, 2018

nertim suggested changes Apr 23, 2018

View reviewed changes

exc3eed added 3 commits April 24, 2018 18:28

Changes based on PR feedback

e03d5d5

Merge branch 'asfarok-envswitcher' of https://github.com/exc3eed/azur…

f75aab5

…e-functions-ux into asfarok-envswitcher

Merge branch 'dev' of https://github.com/Azure/azure-functions-ux int…

192f927

…o asfarok-envswitcher

Azure deleted a comment Apr 25, 2018

Fixing codacy findings

8259e18

Azure deleted a comment Apr 25, 2018

nertim reviewed Apr 25, 2018

View reviewed changes

nertim approved these changes Apr 25, 2018

View reviewed changes

ehamai approved these changes Apr 25, 2018

View reviewed changes

nertim and others added 2 commits April 25, 2018 16:31

Update the scenario status for all environments for AI based monitoring.

ceb201c

merge monitoring fixes

213e320

hartra344 closed this May 21, 2018

exc3eed deleted the asfarok-envswitcher branch August 9, 2018 01:33

		import { Injector } from '@angular/core';




		export class OnPremEnvironment extends Environment {

Bug 11405505:[UR2] Bring Environment Switcher and QuotaSettings API to Functions Portal to support the new Websites blades based on IFrame like AppSettings #2593

Bug 11405505:[UR2] Bring Environment Switcher and QuotaSettings API to Functions Portal to support the new Websites blades based on IFrame like AppSettings #2593

Conversation

exc3eed commented Apr 20, 2018

exc3eed commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exc3eed Apr 23, 2018 • edited

Choose a reason for hiding this comment

ehamai Apr 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exc3eed Apr 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exc3eed Apr 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nertim left a comment

Choose a reason for hiding this comment

jvano commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehamai left a comment

Choose a reason for hiding this comment

nertim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nertim left a comment

Choose a reason for hiding this comment

ehamai left a comment

Choose a reason for hiding this comment

hartra344 commented May 16, 2018

exc3eed Apr 23, 2018 •

edited

ehamai Apr 23, 2018 •

edited

exc3eed Apr 23, 2018 •

edited

exc3eed Apr 23, 2018 •

edited