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

Ability to set mobile view cookie name in root web.config #4064

Merged
merged 18 commits into from
Sep 10, 2020

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Sep 8, 2020

Fixes #4044

Summary

Allows ability to set mobile view cookie name.

I have been having issues with VS + Github so may need new setup to get that part right... please double check things for me if you can test the cookies are able to be set properly when this fix is applied. 0 build errors... but still something can go wrong for someone potentially please be sure this fix is acceptable. I ended up just copying my changes into the GitHub browser... please again double check this will work please test for me if you can find time as I know it is something that needs tested.

Thank you everyone for help @bdukes deserves some credit here for the help thank again.

I was also thinking of changing out the comments which I had added you can edit as you please for any changes you desire.

And also in the same file some obsolete items could be resolved I could potentially help with that as well if or when needed.

Thank you!

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.

Do we want to add the web.config values on upgrade, or just rely on the fallback? Otherwise this looks good (with a few suggestions)

DNN Platform/Website/development.config Outdated Show resolved Hide resolved
DNN Platform/Website/release.config Outdated Show resolved Hide resolved
@bdukes bdukes added this to the 9.7.2 milestone Sep 8, 2020
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

@bdukes I assume we should add a configuration merge file into this as well for upgrades?

@bdukes
Copy link
Contributor

bdukes commented Sep 8, 2020

We can, but if we're setting it to the same value as the default, I don't know that it really matters.

thabaum and others added 10 commits September 8, 2020 08:03
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
@thabaum
Copy link
Contributor Author

thabaum commented Sep 8, 2020

setting it or not it can be added later by someone interested in changing it.

I wanted to use static, with a const the suggestion was to change to readonly to allow variable. I rather have done it your way @bdukes thanks for showing it was ok to do so.

I like the changes and committed them as shown. Thank you.

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
@thabaum
Copy link
Contributor Author

thabaum commented Sep 8, 2020

@bdukes I assume we should add a configuration merge file into this as well for upgrades?

@mitchelsellers I was curious about this part as well, and where or how this would be done for future reference.

It should have a default value no matter if it is set in web.config or not however it is nice to know you can set this and I would welcome it being added to my configs through an upgrade.

If you want it to be added you can point me in the direction it will be good practice.

Should this get documented in the dnndocs.com as well @david-poindexter? I would like to do the SMTP docs if not added for my last changes to host sending test emails. I can document this as well while I am working in that area of documentation if sounds like a plan.

Thank you.

@mitchelsellers
Copy link
Contributor

@bdukes I would tend to agree, my only concern/thought is about the discoverability of the property. Without the configuration value there and no UI reference to it, I don't know that existing sites would know it is an option.

@thabaum
Copy link
Contributor Author

thabaum commented Sep 8, 2020

I agree with the discoverability being an issue which is why I also wish to see it documented.

May I ask which file would be able to manage update changes like this? Just out of curiosity even if we do not add anything to it for this PR.

@bdukes
Copy link
Contributor

bdukes commented Sep 8, 2020

You would add a versioned .config file with an XML Merge document. You can see an example from the Token Provider PR #3969, 09.08.00.config.

@thabaum
Copy link
Contributor Author

thabaum commented Sep 8, 2020

That should make it work please adjust things as needed I am hoping I did this correctly.

Thank you.

@thabaum thabaum requested a review from bdukes September 8, 2020 19:05
@thabaum
Copy link
Contributor Author

thabaum commented Sep 8, 2020

Thought I was done but do you think I should also add the keys to appSettings in the Test App.config file as well?

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.

This all looks good to me. I don't think it makes a difference to set it in the tests' config file.

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.

Looks good to me

@valadas valadas merged commit 1ff653a into dnnsoftware:develop Sep 10, 2020
@thabaum thabaum deleted the patch-28 branch December 7, 2021 03:08
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.

Cookie uses dnn_IsMobile with term "dnn"
4 participants