Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: activate superadmin's all vfolder listing feature #1237

Merged
merged 12 commits into from
Apr 12, 2022

Conversation

SonMinWoo
Copy link
Contributor

@SonMinWoo SonMinWoo commented Feb 20, 2022

resolve: lablup/backend.ai#321
related: lablup/backend.ai-manager#544

When I tested, the function is working.
Trying with the admin accesskey, I get an error. Trying with the folder owner accesskey it succeeds.

issue321_1

But in webui, superadmin can't know all vfolders. This is because superadmin's all folder view function did not work.
It was already implemented in the backend. One parameter required by the backend was missing, so add it.

image

It works well. (The two folders in the picture below are folders created by different accounts as shown in the picture above.)
image

To reduce the load, it request only one-time when superadmin check delegated session start checkbox.
issue321-2

@SonMinWoo SonMinWoo self-assigned this Feb 20, 2022
@SonMinWoo SonMinWoo added the type:fix Fix features that are not working label Feb 20, 2022
@SonMinWoo SonMinWoo requested review from lizable and adrysn February 21, 2022 01:26
Copy link
Contributor

@lizable lizable left a comment

Choose a reason for hiding this comment

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

I have a few things to request. Please refer to the comments below.

@@ -363,11 +363,11 @@ export default class BackendAiResourceBroker extends BackendAIPage {
* Update virtual folder list. Also divide automount folders from general ones.
*
*/
async updateVirtualFolderList() {
async updateVirtualFolderList(userEmail = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The type of default value of parameter userEmail doesn't seem to have a match with SDK function in here.
It's string, not boolean.

const ownerEnabled = this.shadowRoot.querySelector('#owner-enabled');
const userEmail = this.shadowRoot.querySelector('#owner-email').value;
if (ownerEnabled && ownerEnabled.checked) {
await this.resourceBroker.updateVirtualFolderList(userEmail).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for using await keyword and .then() keyword at the same time?

let reqUrl = this.urlPrefix;
if (groupId) {
const params = {group_id: groupId};
if (groupId || userEmail) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to use condition statements separately.

@lizable lizable added this to the 22.03 milestone Feb 23, 2022
Copy link
Contributor

@lizable lizable left a comment

Choose a reason for hiding this comment

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

LGTM

@lizable
Copy link
Contributor

lizable commented Apr 11, 2022

I just updated two things during delegate session creation:

  • Enable the session delegation checkbox only if the owner's keypair is valid.
  • Fetch the owner's accessible vfolder list only if the owner's keypair is valid.

@inureyes
Copy link
Member

@lizable Great!

@inureyes inureyes merged commit 913d1af into main Apr 12, 2022
@inureyes inureyes deleted the fix/delegate-session-vfolder-listing branch April 12, 2022 13:41
@inureyes inureyes added effort:easy Need to understand only a specific region of codes (good first issue, easy). and removed effort:1 labels Mar 27, 2023
@Yaminyam Yaminyam added the size:L 100~500 LoC label Apr 13, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:ux UI / UX issue. effort:easy Need to understand only a specific region of codes (good first issue, easy). size:L 100~500 LoC type:fix Fix features that are not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delegated session creation feature is not working
4 participants