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 issue #4900 - created and used overloads that take the portalId as parameter #4901

Merged
merged 7 commits into from
Nov 9, 2021

Conversation

robsiera
Copy link
Contributor

@robsiera robsiera commented Nov 3, 2021

Summary

I created the necessary overloads that take the portalId as a param.
I changed LoadPortalSettings to use that new overload.

Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great change! It's always good to track down and fix places where the API incorrectly relies on HttpContext (or other globals).

@bdukes bdukes added this to the 9.11.0 milestone Nov 3, 2021
@robsiera
Copy link
Contributor Author

robsiera commented Nov 3, 2021

In my first iteration I duplicated that code to make things very clear. Now I refactored as it would have been my code: I did not touch the public methods interface

Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

This looks good to me, do we need to hold it 9.11.0 or can we get this into 9.10.3 @bdukes ?

@robsiera
Copy link
Contributor Author

robsiera commented Nov 8, 2021

I noticed that I forgot to push one file edit. I marked a method as Obsolete, but I'm not using the new method in my commit. I've just committed that.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Even better!

@bdukes bdukes modified the milestones: 9.11.0, 9.10.3 Nov 9, 2021
@bdukes
Copy link
Contributor

bdukes commented Nov 9, 2021

Once this build is complete, let's squash and merge @valadas

@bdukes bdukes merged commit eed6166 into dnnsoftware:develop Nov 9, 2021
@valadas valadas modified the milestones: 9.10.3, 9.11.0 Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants