Where communities thrive

Join over 1.5M+ people
Join over 100K+ communities
Free without limits
Create your own community

Explore more communities

adaptlearning/tenant-management

https://github.com/adaptlearning/adapt_authoring/milestone/24

People

Repo info

Activity

Dan Gray

@dancgray

That sounds less than ideal. I though that was only for master tenants?

Louise McMahon

@canstudios-louisem

ill raise it on the pr

Louise McMahon

@canstudios-louisem

Ive raised the tenant admin plugin install issue on the pr https://github.com/adaptlearning/adapt_authoring/pull/1637#issuecomment-304897489

Also whats the best way to discuss the db issue, we said it needed further discussion?

Tom Taylor

@taylortom

Excellent, thanks @canstudios-louisem

It may be best to arrange a call, my main reason for halting the discussion today was that @aniketdharia and @ambadasavhad were missing

Failing that, we could always do it in a blog post on the tech forum

Aniket Dharia

@aniketdharia

Hi @taylortom amabadas is struggling with access issue to commit the latest code. Can you resolve it?

Tom Taylor

@taylortom

Hi Aniket, yeah sure, what’s the problem?

Ambadas Avhad

@ambadasavhad

@aniketdharia problem is resolved , @taylortom can you please review the changes

Aniket Dharia

@aniketdharia

oops! Just came in office .. :P tght it is still open...

regarding new requirements.. can we have a short hangouts call @taylortom and @ambadasavhad ?

Tom Taylor

@taylortom

Yeah sure, we suggested as much yesterday

Louise McMahon

@canstudios-louisem

just had a quick look at the latest commit looks good @ambadasavhad are you planing to add some server side checks for if the user has permission to create a user in that tenant as not everything supports disabled and you could bypass that easily anyway?

Ambadas Avhad

@ambadasavhad

@canstudios-louisem sure

Tom Taylor

@taylortom

@canstudios-louisem there should be server-side validation for those routes, returning a 500 or similar (haven’t tested this recently though...)

Louise McMahon

@canstudios-louisem

@taylortom Currently a tenant admin can put someone into a tenant that is not theirs which they shouldnt be able to do thats what I was getting at

Tom Taylor

@taylortom

Ah ok, well that needs looking at then

Louise McMahon

@canstudios-louisem

same should be done for the super admin role aswell

Tom Taylor

@taylortom

@aniketdharia shall we pencil in a catch-up meeting some time this week?

Louise McMahon

@canstudios-louisem

Im starting to struggle to track the issues we have raised for tenant management can i suggest instead of putting them on the pr we raise them as issues and add them to the tenant management milestone?

Dan Gray

@dancgray

:+1: probably would be easier

I think your point about XSS and handlebars should be raised as a seperate issue. It’s something we should address and apply consistently.

Louise McMahon

@canstudios-louisem

I think there should be one issue for the tenant admin one and another for the other areas ill raise them though, it looks like QA has found a few in other areas aswell so it does need looking at.

Dan Gray

@dancgray

:+1:

canstudios-nicolaw

@canstudios-nicolaw

Afternoon all,

We've been working on the tenant management branch here and have a few fixes ready for the todo items on the tenant management PR (adaptlearning/adapt_authoring#1637):

Tenant admin: Able to change the owner of any course in the tenancy
Tenant admin: Should not be able to upload plugins

As part of the work to allow a tenant admin to change the owner of any course in their tenancy the tenant admin can see all courses in their tenant. This means that they can also delete courses in their tenant. However it seems to be the case that shared courses can only be deleted by the project owner - so a small change would be needed here for the following todo item to be checked off "Tenant admin: Able to remove any course in the tenancy".

We have also been working on the naming of a tenant database. Currently the "Tenant name" field is used as the database name. There is a scenario which can cause a database clash:

Create a tenant with name "TenantA", this tenant will have a database of the same name
Change the tenant name, the name of it's database will not change
Create a second tenant with name "TenantA". This tenant will be created because the tenant name is unique, however it will share a database with the existing tenant.

We have made a change so that the tenant id is used as the database name. This way it will always be unique and we don't have to worry about sanitising user input before it is used as the db name or scenarios like the above.

We would like to give these back to the tenant management branch but aren't sure on the best way to do it. Should we create a PR to the branch or just add the changes to it? They are just being tested and should be ready sometime tomorrow.

Thanks

canstudios-nicolaw

@canstudios-nicolaw

^ We have finished testing and are ready to give these back to the tenant management branch as soon as we know the best way to do it.

Dan Gray

@dancgray

@canstudios-nicolaw if it was me I’d make a PR to tenant-management. But I have not been involved in the tenant management work much.

Hope that helps :worried:

BTW shoule creating a non master tenant create a new DB?

canstudios-nicolaw

@canstudios-nicolaw

@dancgray I've noticed that when you first make a tenant its db doesn't appear in the db list. You have to create a user in that tenant and then that user needs to create something which would be stored in the tenant's db (eg. a course).

All users are stored in the master db, so just adding a user to a tenant won't be enough to 'create' the tenant db.

Hope that helps

Aniket Dharia

@aniketdharia

@/all Apologies for missing the call. @ambadasavhad will send the updated PR by this weekend.

Dan Gray

@dancgray

Hi, is anyone else able to run install on the tenant-management branch. I'm getting ENOENT errors on the temp folder.

Ambadas Avhad

@ambadasavhad

Hi @/all For update tenant list dropdown on adding new tenant in add user form , I think we have to make user-management dependable on tenant-management.

any ideas?

Tom Taylor

@taylortom

@ambadasavhad what do you need from tenant management?

Ambadas Avhad

@ambadasavhad

@/all should users from non-master tenant able to upload the plugins?

Tom Taylor

@taylortom

To me, it makes sense to restrict this to super admins only

lc-thomasberger

@lc-thomasberger

I think we should support tenant specific plugins, as it is very likely that each tenant would have different themes.

Louise McMahon

@canstudios-louisem

That was discussed origionally @lc-thomasberger but it was decided it was not MVP we should stick to what we decided earlier

lc-thomasberger

@lc-thomasberger

Yes, I know that it is not part of MVP and that's fine for me. I just think that it is important for tenants to support different plugins. So in the long run we should support it, but not for MVP.

Louise McMahon

@canstudios-louisem

Oh yes i agree its someting Can would need before we could really use multi tennant

lc-thomasberger

@lc-thomasberger

:+1:

Ambadas Avhad

@ambadasavhad

So for this MVP we need to hide either 'plugin management' from global menu or 'upload plugin' from plugin management sidebar for users other than 'super admin'. right?

Louise McMahon

@canstudios-louisem

I think just hide it from the bar if your not a super admin in the master tennant.

What do we want to do with import? curently thats limited to super admins as you can upload plugins via that.

We talked about allowing none super admins to import but only if the course did not need to add a plugin

Tom Taylor

@taylortom

Yeah, did we outline any requirements for import? Can’t remember now

Louise McMahon

@canstudios-louisem

I dont think so as i dont think we expected it to be out first