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

Power Pages vscode.dev web extension schema and local store added #229

Merged
merged 4 commits into from
Jul 8, 2022

Conversation

vedg
Copy link
Contributor

@vedg vedg commented Jul 7, 2022

@@ -0,0 +1,6264 @@
export const portal_schema_data = {
"entities":{
"organization":[
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename this to rootEntity or commonEntity or fallbackEntity
organization is very specific to Dataverse. In case of GitHub it won't be relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought I feel dataSourceProperties would be even more relevant and aligned with our terminologies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,6264 @@
export const portal_schema_data = {
"entities":{
"organization":[
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is same as entities.fields , keepign the schema str intact

* Licensed under the MIT License. See License.txt in the project root for license information.
* ------------------------------------------------------------------------------------------ */

export const PORTAL_LANGUAGES = 'adx_portallanguages';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to have constants other than Dataverse constants then we should have an Enum to group the constants properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will take up in next iteration


try {
const requestUrl = getCustomRequestURL(dataverseOrg, PORTAL_LANGUAGES, PORTAL_LANGUAGES_URL_KEY);
const req = await fetch(requestUrl, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is response, please rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const options = { detail: 'Auth failed in language id code', modal: true };
vscode.window.showErrorMessage("Language code fetch failed", options);
}
const res = await req.json();
Copy link
Contributor

Choose a reason for hiding this comment

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

If the response isn't a JSON this will throw an exception. But our catch block isn't handling that exception.

} catch (e: any) {
if (e.message === 'Unauthorized') {
const options = { detail: 'Auth failed in language id code', modal: true };
vscode.window.showErrorMessage("Language code fetch failed", options);
Copy link
Contributor

Choose a reason for hiding this comment

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

this implementation is being repeated across every API call. Please move it a common function.


try {
const requestUrl = getCustomRequestURL(dataverseOrg, WEBSITE_LANGUAGES, WEBSITE_LANGUAGES_URL_KEY);
const req = await fetch(requestUrl, {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, this response not a request.

const req = await fetch(requestUrl, {
headers: getHeader(accessToken),
});
if (!req.ok) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this implementation is being repeated across every API call. Please move it a common function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}
}
} catch (e: any) {
if (e.message === 'Unauthorized') {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a very specific implementation and I don't think we should look for the exact string match for error handling.

  1. Is this not caught above in the (!req.ok)?
  2. Can this be based on errorCode instead of string comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exception will not be caught in !req.ok, have enhanced this

return { webpagestowebpagesId };
}

export async function setLocalStore(accessToken: any, dataverseOrg: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider renaming this to setContext()


import { portal_schema_data } from "./portalSchema";

export function readSchema() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this function? The only thing it is doing is invoking another function.

return organizationMap
}

export function getOrganizationMap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on the name of the Organizations object in the Schema, please rename these.

break;

setContext
Copy link
Contributor

Choose a reason for hiding this comment

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

is this wrongly put here?

}
else {
showErrorDialog("Error processing the adx_languages response", "Language code response failure");
throw e;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this deliberate? I don't see the calling function handling this exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will remove this in fetch PR

@vedg vedg merged commit e416247 into main Jul 8, 2022
@vedg vedg deleted the users/vedgarbha/schema_0707 branch July 8, 2022 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants