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: Display beta resources subgroups for admin group #2364

Merged
merged 5 commits into from
Jan 31, 2023

Conversation

MaryannGitonga
Copy link
Contributor

Overview

Closes #2356

Demo

image

Notes

Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.

Testing Instructions

  • Checkout to this branch.
  • Load local instance of GE.
  • Navigate to the Resources Explorer tab
  • Toggle the Switch to beta toggle button
  • Observe groups under the admin group

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2364.centralus.azurestaticapps.net

Copy link
Contributor

@Onokaev Onokaev left a comment

Choose a reason for hiding this comment

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

Nice catch. Could we also update the fallback resources.json file. I noticed that it doesn't have the new beta nodes that prompted for this fix.
Get a new copy here:https://graphexplorerapi.azurewebsites.net/swagger/index.html on the openapi/tree endpoint

Onokaev
Onokaev previously approved these changes Jan 30, 2023
Copy link
Contributor

@Onokaev Onokaev left a comment

Choose a reason for hiding this comment

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

Looks good!

@Onokaev Onokaev self-requested a review January 30, 2023 13:28
thewahome
thewahome previously approved these changes Jan 30, 2023
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2364.centralus.azurestaticapps.net

ElinorW
ElinorW previously approved these changes Jan 30, 2023
@darrelmiller
Copy link
Contributor

@thewahome @gavinbarron Why is the resources tree being populated from a static JSON file? This information should come from DevX API. Microsoft Graph is updated weekly. We cannot wait until the next Graph Explorer release to get the latest version of the available resources. We also can't add an 8MB payload to every user of Graph Explorer regardless of whether they look at Resource Explorer or not.

@MaryannGitonga MaryannGitonga dismissed stale reviews from ElinorW, thewahome, and Onokaev via 0d0e4a4 January 30, 2023 17:30
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2364.centralus.azurestaticapps.net

@MaryannGitonga
Copy link
Contributor Author

MaryannGitonga commented Jan 30, 2023

@darrelmiller, the update in the resources.json file is for the cached resources, in case GE cannot fetch the resources from DevX API. The resources in the file were outdated hence why I did an update. In normal cases, the resources should be fetched from the API. And that works fine now.

I am currently looking into a periodic comparison that can be done on the resources.json to check if it's outdated and if that is the case, it is updated with the latest resources. It could be resource-intensive hence why I think it would be a better idea if it were after a significant period of time (eg. a few days). Feel free to share your thoughts on this.

I hope I have answered your question.

@darrelmiller
Copy link
Contributor

Hi @MaryannGitonga Thanks for the clarification. A few things. I don't believe the caching is working as intended. I just went to Graph Explorer, called Microsoft Graph, showed the permissions tab, which returned results from DevX API and still the resource explorer does not show the additional endpoints under admin.

image

Secondly, I don't believe that packaging a cached copy of the resources.json as part of the GE deployment is an effective way to handle DevX API outages. If a customer has no connection, then they will not be able to use Graph Explorer to make calls, so the value of having the resources tab is severely limited. If DevX API is down then I would be perfectly fine in degrading the experience to indicate that the resources tree is not currently available. If we choose to use caching to provide some kind of offline mode, then we should leverage one of the many caches that exist in the browser, we should not be inventing our own.

Adding resources.json to the GE component would significantly increase its download size for every customer, regardless of whether they used the resources tab or not. That doesn't seem customer friendly to me.

I believe this is a similar solution to what we do for samples today. For the same reasons, I don't think we should continue with that approach.

@MaryannGitonga
Copy link
Contributor Author

MaryannGitonga commented Jan 31, 2023

@darrelmiller, this is well noted. I will work on the in-browser caching on a separate PR. Thanks

@thewahome
Copy link
Collaborator

@thewahome @gavinbarron Why is the resources tree being populated from a static JSON file? This information should come from DevX API. Microsoft Graph is updated weekly. We cannot wait until the next Graph Explorer release to get the latest version of the available resources. We also can't add an 8MB payload to every user of Graph Explorer regardless of whether they look at Resource Explorer or not.

@darrelmiller, GE fully depends on the DevX API to get the resources. The JSON was initially used to have a local copy of it so that we can have a cached version to show in case of inconsistencies with the DevX API.

I understand what you are saying about caching. We have a better mechanism when handling the sample queries which we will use here and remove the local JSON files.

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2364.centralus.azurestaticapps.net

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-2364.centralus.azurestaticapps.net

@thewahome thewahome merged commit 7655802 into dev Jan 31, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@gavinbarron
Copy link
Member

@darrelmiller I agree that we should be using DevX API as a primary source, anything else is unacceptable.

Having a fallback to use should we not get a response from DevX API is a good idea in my opinion to prevent providing a poor user experience. We can and should be loading that resource using an awaited import() function, thus the weight is only placed on the browsing experience when DevX API has failed to respond.

Now if we were to leverage some of the browser storage APIs and cache the response from DevX API indefinitely but keep a refresh period to update the cache I'd be totally comfortable with dropping the static JSON file altogether and providing a degraded experience if the user has never loaded GE on that browser and DevX API fails to respond.

@thewahome thewahome deleted the fix/display-beta-resources-nodes-under-admin-node branch February 8, 2023 08:35
@github-actions github-actions bot mentioned this pull request Feb 8, 2023
thewahome pushed a commit that referenced this pull request Feb 13, 2023
Feature: add general playwright tests (#2149)

Task: Add Graph Support Info on README.md (#2347)

Task: Add logging for script errors (#2351)

Task: Add language snippet telemetry (#2371)

Task: Rename sample query "all the items in my drive" to "list items in my drive" (#2350)

Fix: Response area container heights (#2372)

Fix: Re-order tests (#2354)

Fix: Inform user if URL is incomplete (#2367)

Fix: Autocollapsing of history items (#2331)

Fix: Screen blanking on render (#2403)

Fix: Add method to filter ResizeObserver exceptions (#2342)

Fix: enter key runs previous query (#2348)

Fix: autocomplete character selection (#2304)

Fix: Display beta resources subgroups for admin group (#2364)

Fix: Update deprecated aria-label (#2368)

Fix: Default overlay on collections review panel (#2377)

Chore: December dependabot upgrades (#2296)

Chore: Dependabot upgrades February (#2401)

chore: Dependabot upgrades (#2335)

Chore: Code cleanup (#2251)
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.

Resource Explorer and auto complete is not showing some beta endpoints
6 participants