Fix issue in toggle edit mode #4608
Conversation
|
@hishamco I tested the current logic and it does not actually seem to be an "issue"? This line of code is only executed for personalized pages - and the functionality works as expected. The only "problem" appears to be that it sets PageState.EditMode when its not required - however this does not affect the behavior of the application. Please explain if I am missing something based on your own testing. |
| @@ -147,8 +147,7 @@ | |||
| { | |||
| if (PageState.Page.IsPersonalizable && PageState.User != null && UserSecurity.IsAuthorized(PageState.User, RoleNames.Registered)) | |||
| { | |||
| PageState.EditMode = true; | |||
There was a problem hiding this comment.
I'm not sure how you set the EditMode then check it in the line after?!!
There was a problem hiding this comment.
Did you run the application and step into the code with a debugger? When I did this I can see that the PageState.EditMode is set to true, and then the browser is navigated to a Url with a querystring parameter of "?edit=true" - which works fine as that is what the end user is expecting. I am not saying the code is perfect - my question to you was if your testing was resulting in behavior which was causing a functional issue with the application, and if so if you could explain the problem.
There was a problem hiding this comment.
As I said in another PR I'm trying to test the functionality from within the code to improve the quality
There was a problem hiding this comment.
I am not sure what you mean by "testing the functionality from within the code"? Does this mean that you are not actually testing the code at run-time, but instead using static code analyzers to identify potential issues?
There was a problem hiding this comment.
I'm not testing the code at the runtime, sometimes I identifying the bugs by observation or static code analyzers
There was a problem hiding this comment.
Ok it sounds like the answer to my earlier question is that you did not experience any functional problems with the application ie. this is not an "issue" according to the standard definition. This is an optimization to code which is already working properly. In general my philosophy in these cases is "if it aint broke then dont fix it" as there are often unexpected side effects. In this case the change is very straightforward and easy to test/validate so I will merge the PR.
|
@sbwalker shall I start some unit tests that examine several scenarios that might break the code |
No description provided.