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.

Sign up for GitHub

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

Jump to bottom

Bug 1881659: fix guided tour alert to not show up if skipped once #6714

Merged

openshift-merge-robot merged 1 commit into openshift:master from nemesis09:fix-guided-tour-alert

Sep 28, 2020

Contributor

nemesis09 commented Sep 22, 2020

Fixes:
https://issues.redhat.com/browse/ODC-4799

Analysis / Root cause:
Even though the user chooses to skip the guided tour of the dev perspective, the system will nag them again if they happen to follow these steps.

Solution Description:
User should not see guided tour if they have previously skipped or completed it.

Screens:
Before

After

Test Coverage

Browser Conformance

Firefox
Chrome
Safari
Edge

openshift-ci-robot added the bugzilla/severity-medium label

Contributor

openshift-ci-robot commented Sep 22, 2020

@nemesis09: This pull request references Bugzilla bug 1881659, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1881659: fix guided tour alert to not show up if skipped once

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added bugzilla/invalid-bug component/core labels

openshift-ci-robot requested review from jcaianirh and jhadvig

September 22, 2020 20:30

Contributor Author

nemesis09 commented Sep 22, 2020

/bugzilla refresh

openshift-ci-robot added the bugzilla/valid-bug label

Contributor

openshift-ci-robot commented Sep 22, 2020

@nemesis09: This pull request references Bugzilla bug 1881659, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot removed the bugzilla/invalid-bug label

sahil143 suggested changes

View reviewed changes

frontend/packages/console-app/src/components/tour/tour-context.ts Outdated

                 }
               };
-              type TourReducer = Reducer<TourState, TourActions>;
+              type TourReducer = Reducer<TourState, { type: TourActions; payload?: boolean }>;

Contributor

sahil143 Sep 23, 2020

Probably introduce a new for action say

type TourStateAction = { type: TourActions; payload?: { completed?: boolean }}

Suggested change

      
            type TourReducer = Reducer<TourState, { type: TourActions; payload?: boolean }>;
          
            type TourReducer = Reducer<TourState, TourStateAction>;

would be better to make payload an object and with a completed prop. So it's more readable and we know what we are sending in the payload. In future we might need to send some other data inside payload.

Contributor Author

nemesis09 Sep 23, 2020

done

frontend/packages/console-app/src/components/tour/tour-context.ts Outdated

-                switch (action) {
+                switch (action.type) {
+                  case TourActions.initialize:
+                    return initializeState(action.payload);

Contributor

sahil143 Sep 23, 2020

no need to declare a method here. just set the state directly here

Suggested change

      
                  return initializeState(action.payload);
          
                  return {
          
              completedTour: completed,
          
              stepNumber: 0,
          
              startTour: !completed,
          
            };

Contributor Author

nemesis09 Sep 23, 2020

done

frontend/packages/console-app/src/components/tour/tour-context.ts Outdated

                     setTourCompletionLocalStorageDataForPerspective(activePerspective, true);
                   }
                 };
+                const activePerspectiveRef = useRef(activePerspective);

Contributor

sahil143 Sep 23, 2020

Suggested change

      
              const activePerspectiveRef = useRef(activePerspective);
          
              const activePerspectiveRef = useRef<string>(activePerspective);

Contributor Author

nemesis09 Sep 23, 2020

done

nemesis09 force-pushed the fix-guided-tour-alert branch from 92c0a3f to 650afaf Compare

September 23, 2020 20:53

christianvogt reviewed

View reviewed changes

frontend/packages/console-app/src/components/tour/tour-context.ts Outdated

Comment on lines 100 to 103

+                if (activePerspective !== activePerspectiveRef.current) {
+                  tourDispatch({ type: TourActions.initialize, payload: { completed } });
+                  activePerspectiveRef.current = activePerspective;
+                }

Contributor

christianvogt Sep 23, 2020

You should execute side effects inside useEffect

Suggested change

      
              if (activePerspective !== activePerspectiveRef.current) {
          
                tourDispatch({ type: TourActions.initialize, payload: { completed } });
          
                activePerspectiveRef.current = activePerspective;
          
              }
          
            React.useEffect(() => {
          
                tourDispatch({ type: TourActions.initialize, payload: { completed } });
          
                setPerspective(activePerspective);
          
              }, [activePerspective, completed]);

Contributor Author

nemesis09 Sep 23, 2020

used useEffect for initializing state on perspective change

frontend/packages/console-app/src/components/tour/tour-context.ts Outdated

                     setTourCompletionLocalStorageDataForPerspective(activePerspective, true);
                   }
                 };
+                const activePerspectiveRef = useRef<string>(activePerspective);

Contributor

christianvogt Sep 23, 2020

Suggested change

      
              const activePerspectiveRef = useRef<string>(activePerspective);
          
              const perspective = React.useState(activePerspective);

Contributor Author

nemesis09 Sep 23, 2020

done

nemesis09 force-pushed the fix-guided-tour-alert branch from 650afaf to 56eb1be Compare

September 23, 2020 21:38

sahil143 reviewed

View reviewed changes

frontend/packages/console-app/src/components/tour/tour-context.ts Outdated

+                useEffect(() => {
+                  tourDispatch({ type: TourActions.initialize, payload: { completed } });
+                  setPerspective(activePerspective);
+                }, [activePerspective, completed]);

Contributor

sahil143 Sep 24, 2020

I don't think we need to run the effect on completed. We want to only run the effect when the perspective change so that the tour state can be initialized.

Suggested change

      
              }, [activePerspective, completed]);
          
              }, [activePerspective]);

Contributor Author

nemesis09 Sep 24, 2020

updated dependency and disabled es lint rules for the line

nemesis09 force-pushed the fix-guided-tour-alert branch from 56eb1be to 935cf67 Compare

September 24, 2020 16:09

sahil143 approved these changes

View reviewed changes

Contributor

sahil143 left a comment

/lgtm

openshift-ci-robot assigned sahil143

openshift-ci-robot added the lgtm label

Contributor

christianvogt commented Sep 24, 2020

Going from admin -> dev perspective when the dev perspective tour has already been completed still shows the tour.
/lgtm cancel

openshift-ci-robot assigned christianvogt

openshift-ci-robot removed the lgtm label

nemesis09 force-pushed the fix-guided-tour-alert branch from 935cf67 to 443b32a Compare

September 25, 2020 21:07

Contributor Author

nemesis09 commented Sep 25, 2020

Going from admin -> dev perspective when the dev perspective tour has already been completed still shows the tour.
/lgtm cancel

@christianvogt pushed changes addressing the issue. please take a look


          fix guided tour alert to not show up if skipped once

23b8895

nemesis09 force-pushed the fix-guided-tour-alert branch from 443b32a to 23b8895 Compare

September 25, 2020 21:54

Contributor

christianvogt commented Sep 28, 2020

/lgtm
/approve

openshift-ci-robot added the lgtm label

Contributor

openshift-ci-robot commented Sep 28, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, nemesis09, sahil143

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the approved label

openshift-merge-robot merged commit 1f26ca5 into openshift:master

Contributor

openshift-ci-robot commented Sep 28, 2020

@nemesis09: All pull requests linked via external trackers have merged:

openshift/console#6714

Bugzilla bug 1881659 has been moved to the MODIFIED state.

In response to this:

Bug 1881659: fix guided tour alert to not show up if skipped once

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

spadgett added this to the v4.6 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment