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

Add a feature to allow the user to manage tenants #12360

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Sep 9, 2022

Add a new feature that would allow the host to manage tenant's features
Fix #11870

This PR enhanced the OrchardCore.Features module by allowing to accept {tenant?} is the route. This tenant value would only be evaluated if the request is running on the host. When a tenant value is present, we'll execute the code in the scope of the given tenant.

Here is a screen shot from the Tenants list in the host the "Manage Features" button was added
image

listing features for site 1 example
image

This is how the features show up when it is running for a tenant and not the host "no changes to the outcome"
image

@jtkech do to multiple conflicts in PR #11869 I decided it was easier to fix it using this PR instead.

@ns8482e
Copy link
Contributor

ns8482e commented Sep 9, 2022

@MikeAlhayek If my memory serves well - I believe it was decided that we reuse the same Feature Management UI and provide tenant selection dropdown

@MikeAlhayek
Copy link
Member Author

@ns8482e yes we decided to reuse the same UI as the code here does. Then we decided to leave the drop down menu into a separate PR. Once this PR is merged, I'll submit a second PR for that.

Copy link
Contributor

@Skrypt Skrypt left a comment

Choose a reason for hiding this comment

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

I approve because as of now we don't have anything else that allows doing this. We may revisit this feature later on by first all agreeing on a concept.

@jtkech
Copy link
Member

jtkech commented Oct 13, 2022

Okay, need to have the code under my eyes and try it, will do it as soon as possible.

@MikeAlhayek
Copy link
Member Author

@jtkech have you had a chance to test this out?

@jtkech
Copy link
Member

jtkech commented Oct 19, 2022

I'm going to look at it

@jtkech
Copy link
Member

jtkech commented Oct 19, 2022

Just fetched your branch to start some tests.

  • Just for info, minor things but Visual studio asked me to install some sdks, didn't investigate yet. Then the new .Features.Core project was not in the right solution folder.

  • Then I created a Tenant1 and saw the new related Manage Features button. Maybe a shorter name as Features, maybe manage this from the .Features page itself where you could select any tenant.

    Note: I also updated the OC.Tenants page, hope my PR will get merged first ;)

  • Then from the .Tenants page I clicked on Manage Features that directed me to the .Features page of Tenant1, okay. Then I enabled the GraphQL feature and was redirected to the .Features page of the Default tenant, hmm here I thought that I would be redirected to the original .Tenants page.

    But the problem for now is that it didn't enable/disable GraphQL in Tenant1 but in the Default Tenant.

@MikeAlhayek
Copy link
Member Author

Are you sure you forked the right branch? These issues were fixed a while ago. I'll check it out again tomorrow

@jtkech
Copy link
Member

jtkech commented Oct 19, 2022

Yes, It seems that I didn't get for example the last Features.cshtml

@jtkech
Copy link
Member

jtkech commented Oct 19, 2022

Okay found it, that's why I needed to install old sdks :(

@jtkech
Copy link
Member

jtkech commented Oct 19, 2022

Okay better, I can now see the smaler Features button.

Now the problem is that if we enable/disable GraphQL in Tenant1 or Tenant2.

It is also enabled/disable in the Default tenant.

I will look at the code tomorrow

@MikeAlhayek
Copy link
Member Author

@jtkech I'll check it again tomorrow. What you are stating should not be the case. if you manage features for tenant1, then you should be in the scope of tenant1.
I know I tested it and @Skrypt also tested it. I suggest you clean your project and run it again just to ensure that you have clean environment. I do another round of validation myself tomorrow to make sure I am not missing something too.

@jtkech
Copy link
Member

jtkech commented Oct 19, 2022

Okay I understand, when I changed the state of the feature of Tenant1, then to check the state of the same feature in the Default tenant I cliked on Features from the left Admin Menu, but in fact it still renders the Feature Page in the context of the Tenant1.

I think because it still has the tenant=Tenant1 as an ambient route value, which maybe would need to be cleared when clicking this admin menu item.

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Oct 19, 2022

I think because it still has the tenant=Tenant1 as an ambient route value, which maybe would need to be cleared when clicking this admin menu item.
Any idea how that would be cleared? Do we just need to add empty string for the tenant parameter in the menu link?

@jtkech
Copy link
Member

jtkech commented Oct 19, 2022

In the AdminMenu.cs

.Action("Features", "Admin", new { area = "OrchardCore.Features", tenant = String.Empty })

@MikeAlhayek
Copy link
Member Author

@jtkech I just push the fix for the menu item. Here is a screen cast.

features

@MikeAlhayek MikeAlhayek merged commit be4fe36 into OrchardCMS:main Oct 20, 2022
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.

Ability to enable/disable feature for Tenant from Host
4 participants