-
Notifications
You must be signed in to change notification settings - Fork 0
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
Problem 10: UseDbScopeAttribute was replaced by InteractionScopeAttribute. #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some change requests and suggestions. Really nice work. Thanks for the thorough testing.
Starcounter.Startup/Routing/Middleware/InteractionScopeAttribute.cs
Outdated
Show resolved
Hide resolved
Starcounter.Startup/Routing/Middleware/InteractionScopeAttribute.cs
Outdated
Show resolved
Hide resolved
Starcounter.Startup/Routing/Middleware/InteractionScopeMiddleware .cs
Outdated
Show resolved
Hide resolved
|
||
namespace Starcounter.Startup.Routing.Middleware | ||
{ | ||
public class InteractionScopeMiddleware : IPageMiddleware |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Will the
DbScope
attribute still work? - Do we use it anywhere?
- If yes, can we mark it obsolete and implement its logic in
InteractionScopeMiddleware
? - If no, can we drop it?
- If yes, can we mark it obsolete and implement its logic in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtornwall the answers:
- Before changing
UseDbScope
onInteractionScope
attribute i made few searches:
I found usage of theUseDbScope
attribute in the 6 apps: https://github.com/search?q=org%3AStarcounter+UseDbScope&type=Code, but at the same time i found invocation of theAddDbScopeMiddleware
(that registers related middleware IIUC) only inAssortmentManager
: https://github.com/search?q=org%3AStarcounter+AddDbScopeMiddleware&type=Code (and inStarcounter.Startup
itself). - So my plan was: simultaneously with replacing old attribute with new one make the replacement in these apps, and invoke method
AddInteractionScopeMiddleware
in all, to restore initial logic. This why i decided to add the case with returning justnext()
as it was in the old attribute (we are discussing this point above currently).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kegor Sounds like a reasonable plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note though, when you upgrade the Startup version used by the HeadsOmni apps (and others), we'll need to do the same for the Buildcraft apps that use it. I can take care of that if you tell me the version to upgrade to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtornwall, seems like improvements by your comments were made, could we go further with merge or some new tests should be made, what do you think?
@@ -0,0 +1,8 @@ | |||
namespace Starcounter.Startup.Routing.Enums |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a separate namespace to hold the enums.
@@ -1,17 +1,11 @@ | |||
using System; | |||
using Starcounter.Startup.Routing.Enums; | |||
|
|||
namespace Starcounter.Startup.Routing.Middleware | |||
{ | |||
[AttributeUsage(AttributeTargets.Class)] | |||
public sealed class InteractionScopeAttribute : Attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should go in the same namespace as the Url
attribute I think.
After discussion with @mtornwall i made following test:
|
Problem #10:
InteractionScopeAttribute
needs to be added.Solution:
In this PR were added
InteractionScopeAttribute
andInteractionScopeMiddleware
, using them (in the current iteration) we can return untouched response orDb.Scope(next)/new Transaction.Scope(new)
depending on attribute mode.Related task: #5.
Tests:
Starcounter.Startup
was added into HeadsOmni sub-modules:SignIn/UserAdmin/Products/Showroom/Blending
.HeadsOmni
solutionbuild.cake
andsetup.cs
were changed to run only apps from p.1.Product page
was added input that showsProducts.Product.Name
value.[InteractionScope(InteractionScopeAttribute.InteractionScopeMode.AttachOrCreate)]
(default)[InteractionScope(InteractionScopeAttribute.InteractionScopeMode.AlwaysCreate)]
on theProductPage
, results on the screenshots below.Screenshots:
![shared_scope](https://user-images.githubusercontent.com/17431807/54603274-f33bc680-4a54-11e9-8690-6d0baa63e535.gif)