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

Can Get Shell By Case Insensitive Name. #12120

Conversation

MikeAlhayek
Copy link
Member

Fix #12106

@jtkech this PR replaces #12114 as per your request

src/OrchardCore/OrchardCore/Shell/ShellHost.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Shell/ShellHostTests.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Shell/ShellHostTests.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Shell/ShellHostTests.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Shell/ShellHostTests.cs Outdated Show resolved Hide resolved
@MikeAlhayek MikeAlhayek requested a review from jtkech July 31, 2022 04:52
@jtkech
Copy link
Member

jtkech commented Jul 31, 2022

@CrestApps

Okay will work on it tomorrow.

Just did some little tweaks locally for the unit tests,

Will push it tomorrow to let you see them.

@hishamco
Copy link
Member

@CrestApps why you closed #12114 it is not a good idea to close a PR and replace it unless there's a value. I had seen a lot of comments in the previous one during the review. Hope to consider this next time and thanks again for your contribution

@MikeAlhayek
Copy link
Member Author

I did that based on the request from @jtkech

Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

I will leave the Shell related stuff to @jtkech, just a little notes about the unit tests

test/OrchardCore.Tests/Shell/ShellHostTests.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Shell/ShellHostTests.cs Outdated Show resolved Hide resolved
test/OrchardCore.Tests/Shell/ShellHostTests.cs Outdated Show resolved Hide resolved
@jtkech
Copy link
Member

jtkech commented Jul 31, 2022

@hishamco

I already said to @CrestApps that I will update the tests with some suggestions, so maybe was too early to review them. The goal being to keep this PR as clean as possible because it acts on a sensitive area.

Just did some little tweaks locally for the unit tests,
Will push it tomorrow to let you see them.

But that's okay as we still only have 2 files changed, so the PR is still "clean" ;)

I will do my suggestions soon.

Anyway the main point is to make _shellContexts and _shellSettings dictionaries case insensitive, I just need to think about it a little more and if I still think it is safe I will merge it (and if unit tests still pass).

Otherwise will wait for more feedback.

@jtkech
Copy link
Member

jtkech commented Jul 31, 2022

@CrestApps

I pushed my suggestions, feel free to revert or update what you don't like ;)

@hishamco
Copy link
Member

Seems the build is failed

@jtkech
Copy link
Member

jtkech commented Jul 31, 2022

I saw it, I will fix it

Fail intermitently while running all tests in parallel and only for the Default tenant

@jtkech jtkech changed the title Add support for case-insensitive tenant names in ShellHost. Can Get Shell By Case Insensitive Name. Aug 1, 2022
@jtkech jtkech merged commit 12fcd58 into OrchardCMS:main Aug 1, 2022
@MikeAlhayek MikeAlhayek deleted the AssSupportForCaseInsensitiveTenantNameInShellHost branch August 1, 2022 16:30
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.

Allow case-insensitive tenant names in ShellHost
3 participants