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

Fixes always populate site settings information correctly #3515

Conversation

tingung
Copy link
Contributor

@tingung tingung commented Jan 21, 2020

Fixes #3514

Summary

Root cause analysis:

  1. component (app.jsx) will always send a new request to update portalId in regardless initialization using utilitities.loadPanel or page refresh - https://github.com/dnnsoftware/Dnn.Platform/blob/develop/Dnn.AdminExperience/ClientSide/Sites.Web/src/_exportables/src/Components/ListView/index.jsx#L52
  2. All the site settings component will always listen to dispatch update for getPortalSettings and will update the props with the state information (received from getPortalSettings patch)
  3. When user click "Site Settings", it will invoke utilities.loadPanel to create the component with portalId. All the components created from component contains the required portalId. However, all the component trying to update the props portalId with the state.siteInfo.portalId. At first, all the component will render correctly but https://github.com/dnnsoftware/Dnn.Platform/blob/develop/Dnn.AdminExperience/ClientSide/SiteSettings.Web/src/components/App.jsx#L135 will dispatch the new getPortalSettings with props.portalId (this props.portalId is always default site portalId - I didn't further investigate the reason behind). Then, all the component will receive the update and override the props based on the state changes.
  4. If we put a break point, we will observe the site settings changed to selected site and subsequent back to default site.

Solution: component should not required to dispatch the getPortalSettings information because onSetting will always pass the correct portalId. Therefore, all components doesn't required to change the portal id props. However, it will cause the problem if Site Settings is loaded directly (user go to Persona Bar > Site Settings). The portal id and culture code is undefined because component is no longer dispatch getPortalSettings to get the portal id and culture code. Therefore, all the components will load the data with undefined portal id and culture code. Most of the components are working except Synonym and Ignored Words because Synonym and Ignored Words API doesn't handle undefined culture code properly and will throw exception if culture code is null. To handle this scenario, I need to update Synonym and Ignored Words API to assumed undefined culture code is means current site default culture code.

Changes:

Server Side API

  • Use default portal locale if culture code is undefined

Client Side
Remove code to override the props.portalId based on the state update of site info

  • props.portalId should always changed by the parent component to allow
    consistency of the portal id when load the component
  • Remove code to dispatch the update of portal id or culture code during
    initialization of App component - if portal id or culture code is
    undefined, allow component to send undefined values to server and return
    default settings - this is to handle initialization of the component
    using page refresh when portal id is undefined
  • if invoke through on settings (Site Settings page) or reload website (drop down selection), then render the component based on portal id from parent component

Manual steps to test

  1. View Site Settings upon page refresh and the site settings shown always intact for the first and subsequent time.
  2. Click on all site settings tab
  3. Switch the site with the drop down and site settings is reflecting correctly.
  4. Switch to different site and test 1 - 3 randomly.

Notes: There is one bug related to site aliases. Site aliases doesn't updated upon changing the site with the drop down (will fix in another PR).

Confirmation video: https://drive.google.com/file/d/1IIi9h6crXk-f5B5-UWgqFwgNU9nn0y4X/view?usp=sharing

state update of site info
- props.portalId should always changed by the parent component to allow
consistency of the portal id when load the component
- Remove code to dispatch the update of portal id or culture code during
initialization of App component - if portal id or culture code is
undefined, allow component to send undefined values to server and return
default settings - this is to handle initialization of the component
using page refresh when portal id is undefined
- if invoke through on settings (Site Settings page) or reload website (drop down selection), then render the component based on portal id from parent component
@tingung
Copy link
Contributor Author

tingung commented Jan 21, 2020

Dear reviewers,
The issue is quite tricky and I have spends hours to debug (lack of the react knowledge and the concept of connect is really painful to debug).
If you have any queries, please feel free to ask. And the best if you can test it from your site even I have tested quite many times locally.

@tingung tingung requested a review from donker January 22, 2020 09:14
- Gets the locale included in the portal if culture code is not null or
empty or else gets the current locale for current request to the portal.
@mitchelsellers
Copy link
Contributor

@valadas can I get your review on this? @mtrutledge you as well?

Copy link
Contributor

@mtrutledge mtrutledge left a comment

Choose a reason for hiding this comment

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

Should we be removing the portal id here? Maybe we should be figuring out why getPortalSettings is always returning a default portal id and not getting the currently selected site. @tingung Do you have any ideas why this is not getting the currently selected sites portal settings.

@tingung
Copy link
Contributor Author

tingung commented Jan 29, 2020

Should we be removing the portal id here? Maybe we should be figuring out why getPortalSettings is always returning a default portal id and not getting the currently selected site. @tingung Do you have any ideas why this is not getting the currently selected sites portal settings.

Hi @mtrutledge , I have further investigated the reason component is always created with current selected site portal id and culture code. component is created by Root.prod.js
https://github.com/dnnsoftware/Dnn.Platform/blob/develop/Dnn.AdminExperience/ClientSide/SiteSettings.Web/src/containers/Root.prod.js#L9.

        let culture = window.parent["personaBarSettings"]["culture"];
        let portalId = window.parent["personaBarSettings"]["portalId"];
        return (
            <div className="siteSettings-Root">
                <App cultureCode={culture} portalId={portalId} />
            </div>
        );

It is getting persona bar culture code and portal id to create component. Based on existing code, https://github.com/dnnsoftware/Dnn.Platform/blob/develop/Dnn.AdminExperience/ClientSide/SiteSettings.Web/src/components/App.jsx#L135 will dispatch the new getPortalSettings with props.portalId which is always current persona bar portal id and culture code.

I believe this code is written such to handle scenario when user go to Persona Bar > Site Settings . It should render the component with current site portal id and culture code.

By removing this code from component,

        if (state.portalId !== props.portalId && state.cultureCode !== props.cultureCode) {
            this.changePortalIdCultureCode(props.portalId, props.cultureCode);
        }
        else if (state.portalId !== props.portalId) {
            this.changePortalId(props.portalId);
        }
        else if (state.cultureCode !== props.cultureCode) {
            this.changeCultureCode(props.cultureCode);
        }

it prevent the component to overwrite the component created by onSettings (listening to the getPortalSettings dispatch). However, it will cause problem when user access Persona Bar > Site Settings directly because all the component created won't have portal Id. There is no getPortalSettings dispatch after the code removal. Therefore, I have to fix the server side to handle scenario for portal id is undefined. If it is undefined, server side assuming user is trying to get current site setting. This behavior was implemented for all setting except synonym and languages which I fix as part of the code.

There is another possibility not to remove above code but differentiate in between component created from utilities.loadPanel (no need to trigger change portal id) or through direct html loading which required. But I think it is harder to maintain rather than rely on behavior that portal id is undefined then always load current site setting.

Anyway, I didn't fully check getPortalSettings dispatch event. I choose to remove component code which rely on state change (possible from getPortalSettings) to update the props portal id. The props portal id is loaded correctly upon onSetting here - https://github.com/dnnsoftware/Dnn.Platform/blob/develop/Dnn.AdminExperience/ClientSide/Sites.Web/src/_exportables/src/Components/ListView/index.jsx#L28

I have tested portal id changed with Setting icon at Persona Bar > Sites page and the drop down of the Site Selection.

@tingung tingung changed the title Fix always populate site settings information correctly Fixes always populate site settings information correctly Jan 29, 2020
@valadas valadas added this to the 9.5.1 milestone Feb 18, 2020
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 and tested, it solves the issue

@mitchelsellers mitchelsellers merged commit 2b3d0bb into dnnsoftware:develop Feb 18, 2020
@valadas valadas modified the milestones: 9.5.1, 9.6.0 Apr 14, 2020
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.

At Persona Bar > Sites, view site settings of non default site always shown default site settings
5 participants