fix(routing): prevent side-effects of action outlet for add-work-item and add-codebases #2794

aptmac · 2018-04-24T21:43:53Z

By fixing up the side-effects that named router outlets are currently causing, this PR accomplishes two things:

Makes the add-work-item action into a separate page

Previously, the add-work-item action would display an "overlay", and cause a 404 when trying to go to the planner, settings, or create page while active [0]. Now, the overlay can be treated as it's own page, which maintains the routing we would expect.

**Note, at the time of writing this something is wrong in the f8-planner side of the add-work-item [1], specifically trying to retrieve the work item types is returning a 404; it looks like it may get amended by [2] . There is an issue logged for it here: [3].

Removes the add-codebases action from the dashboard, and modifies the header component so that switching between the codebases/pipelines/deployments pages with the outlet active will update the header accordingly.

The add-codebases action, similar to the add-work-items action, has routing problems when used in the dashboard. To patch this as discussed in [0] the add-codebases action has been replaced with the code wizard. On the create/ pages however, I patched up the url verification for determining active menu items, so the add-codebases action can still work in codebases and update the header accordingly.

[0] openshiftio/openshift.io#3021
[1] https://github.com/fabric8-ui/fabric8-planner/blob/fa4c909b8297f60e29b5d67dfd43db0e8f0850e4/src/app/components/work-item-create/work-item-create-selector/work-item-create-selector.component.html
[2] fabric8-ui/fabric8-planner#2574
[3] openshiftio/openshift.io#3300

fabric8cd · 2018-04-27T14:10:34Z

@aptmac fabric8/fabric8-ui:SNAPSHOT-PR-2794-2 fabric8-ui is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2794-fabric8-ui.badger.fabric8.io

dlabrecq · 2018-04-28T01:37:56Z

src/app/dashboard-widgets/add-codebase-widget/add-codebase-widget.component.html

-          <button id="spacehome-codebases-create-button" class="btn btn-primary btn-lg" (click)="addToSpace.emit()">Add Codebase</button>
-        </div>
-        <div class="f8-blank-slate-main-action">
-          <button id="spacehome-codebases-import-button" class="btn btn-primary btn-lg" [routerLink]="[{ outlets: { action: 'add-codebase' } }]">Import Codebase</button>


Why was the import codebase button removed?

FYI, @mindreeper2420

dlabrecq · 2018-04-28T01:39:13Z

src/app/dashboard-widgets/add-codebase-widget/add-codebase-widget.component.html

-          <button id="spacehome-my-codebases-create-button" class="btn btn-primary btn-lg" (click)="addToSpace.emit()">Add Codebase</button>
-        </div>
-        <div class="f8-blank-slate-main-action">
-          <button id="spacehome-my-codebases-import-button" class="btn btn-primary btn-lg" [routerLink]="[{ outlets: { action: 'add-codebase' } }]">Import Codebase</button>


Similarly, why was this removed from the old widget?

I think this is touched in the long discussion of openshiftio/openshift.io#3021

openshiftio/openshift.io#3100 (comment)
suggests import codebase functionality can be removed.

On 3021: Alex says:

"Additionally, if we want a fix in place for summit but haven't fully hashed out the details of url patterns, would it be acceptable to make a patch that removes the add-codebases action (or replace it's functionality with displaying the forge-wizard), and have the add-work-item "overlay" be it's own page?"

Joshua replies

"I think it would be. That would at least not break user flow."

So I think these are intentional removals. Thoughts?

I still think if you can't fix it fast, then removing it for now is best.

@catrobson @qodfathr do you agree?

+1 that Add Codebase should go to Launcher Experience (if that flag is enabled for that user on the button) or to the old Forge wizard (if the feature flag is not enabled for that user on the button). If we can't go to the old forge wizard, removing the button from the old widget would be acceptable to me over keeping the old 'create codebase' fly-out panel. @dlabrecq does this make sense to you - I want to make sure I'm understanding this correctly.

There are two issues here; the "import codebase" button and the "add work item" icon.

It's still not clear as to why the "import codebases" button was removed because it still works for me without any changes. If the team wants to remove the "import codebase" button, I'm fine with that. I understand that it's not needed with the new app-launcher; however, that's also being handled by PR #2839. The new app-launcher is shown by a feature flag, so the original card was expected to remain the same.

Regarding the "add work item" icon, I think that should be removed -- at least, for summit. Even with these changes, the user cannot create a work item because two APIs no longer exist. I was able to work around one of them, but not the second missing API. Although, we can try to address that in a separate PR.

I think the issue with the "import codebase" is that if you use it and then route to pipelines or deployments, your URL looks something like:

https://openshift.io/[email protected]/arstneio/create/(pipelines//action:add-codebase)

and the header menu then highlights the Codebases tab even when you're on Pipelines or Deployments. However, as far as I can tell, you're right, everything else works.

That functionality looks okay to me. When I navigate elsewehere, after clicking the "add codebase" icon, the app-routing.module removes the outlet from the URL.

Seems we didn't have these routing issues until the two router-outlets were added to app-component.html?

Huh; I really can't say re. the router outlets being added and these issues being seen. That's another thing to investigate I guess.

dlabrecq · 2018-04-28T01:44:42Z

...dgets/create-work-item-widget/create-work-item-overlay/create-work-item-overlay.component.ts

@@ -43,7 +45,7 @@ export class CreateWorkItemOverlayComponent implements OnInit, AfterViewInit {
  }

  onClose() {
-    this.router.navigateByUrl(removeAction(this.router.url));
+    this.location.back();


When choosing to create a work item, I encounter the error below. I don't see the overlay, but an empty page that says "Choose a new work item type".

ERROR Error: ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'false'. Current value: 'true'.

Both of those exist currently, the logged error isn't displayed in production however.

The first bug is noted here: openshiftio/openshift.io#3300

And the second is noted here: fabric8-ui/fabric8-planner#2571

dlabrecq · 2018-04-28T01:47:38Z

src/app/dashboard-widgets/work-item-widget/work-item-widget.component.html

@@ -12,7 +12,7 @@ <h3>Create Work Items</h3>
      </p>
      <div class="f8-blank-slate-main-action">
        <button id="spacehome-workitems-create-button" class="btn btn-primary btn-lg"
-                [routerLink]="[{ outlets: { action: 'add-work-item' } }]">Create work item</button>
+                [routerLink]="['add-work-item']">Create work item</button>


When choosing to create a work item from the new work-item-widget, I encounter the same error below. The same empty page displays with "Choose a new work item type" and nothing more.

ERROR Error: ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'false'. Current value: 'true'.

Both of those exist currently, the logged error isn't displayed in production however.

The first bug is noted here: openshiftio/openshift.io#3300

And the second is noted here: fabric8-ui/fabric8-planner#2571

jiekang · 2018-04-30T15:41:12Z

The add work item page with this patch is blank. It exposes a component from the Planner project:

  <create-selector-widget #detailAddTypeSelector [workItemTypes]="workItemTypes | async" (onSelect)="onSelect($event)" (onClose)="onClose()"></create-selector-widget>

There is an internal *ngFor that has a null array.

Edit: Oh, looks like this blank page is occurring on production as well. @dlabrecq was that what happened before? Sorry I'm jumping into this with very little background info.

jiekang · 2018-04-30T15:56:28Z

The request to get workitemtypes is 404 for me:

https://api.openshift.io/api/spaces/<id>/workitemtypes

jiekang · 2018-04-30T16:18:12Z

Looks like that endpoint no longer exists. The code making the request is in Planner. Hmm...

This also affects production and is unrelated to this PR.

jiekang · 2018-04-30T16:24:15Z

So after some investigation and discussion with Alex, pretty much all the problems we're seeing have to do with the planner create-selector-widget component we import. As long as this PR fixes the 404 issue it should be okay. It would be great if the planner component worked but I think they're aware and it's on their todo? I'll try to investigate that. Fixing the planner component to work is a separate issue.

joshuawilson · 2018-04-30T16:27:36Z

@dlabrecq can you please take a look at the conflicts since it looks like they came from the changes to the dashboards widgets.

dlabrecq · 2018-04-30T17:11:36Z

There appears to be a different, valid URL we can use with the fabric8-create-work-item-widget:

this.workItemTypes = this.workItemService.getWorkItemTypes2(this.space.relationships.workitemtypes.links.related);

With this in place, I can click on the "add work item" icon and navigate to the planner. However, I did encounter the fabric8-ui/fabric8-planner#2571 issue describe above.

dlabrecq · 2018-05-01T02:23:18Z

FYI, I was able to use a different API (above) so that the create-work-item-overlay displays work item types without errors. I even have a fix for the “Expression has changed” error: fabric8-ui/fabric8-planner#2571

See: #2843 for changes

Unfortunately, I discovered a second invalid API. When clicking on any of the work item types, I see a 404 error for this API:
https://api.openshift.io/api/namedspaces/[email protected]/prod-5/workitems/new?type=26787039-b68f-4e28-8814-c2f93be1ef4e

I have not found an alternative API for that one, yet?

jiekang · 2018-05-01T14:34:24Z

@dlabrecq Thanks so much for your help here!

It looks like with #2839 this area is going to be touched quite heavily. The primary goal of this PR is to prevent a 404 Not Found from occurring when trying to use these actions. This PR achieves that goal. It's unfortunate that one of the actions is currently broken, making it hard to test but I do believe that a separate PR is best to solve that as they are separate issues in the same area of code. Dan has #2843 for that already, which is great, even if it's WIP.

Are there still objections to merging this PR that haven't been addressed? I'd appreciate repeating them so I can do my best to address them!

jiekang

Looks like a unit test is failing:

SUMMARY:

✔ 653 tests completed

ℹ 7 tests skipped

✖ 1 test failed



FAILED TESTS:

  HeaderComponent

    #formatUrl

      ✖ should remove an action outlet from the end of the url

        PhantomJS 2.1.1 (Linux 0.0.0)

      Expected 'create' to equal 'create/'.

      config/spec-bundle.js:287885:35

      invoke@config/spec-bundle.js:225288:31

      onInvoke@config/spec-bundle.js:228189:45

      invoke@config/spec-bundle.js:225287:40

      run@config/spec-bundle.js:225038:49

      config/spec-bundle.js:228397:37

      config/spec-bundle.js:4172:11

      config/spec-bundle.js:56880:31

      invoke@config/spec-bundle.js:225288:31

      onInvoke@config/spec-bundle.js:228487:45

      onInvoke@config/spec-bundle.js:228186:47

      invoke@config/spec-bundle.js:225287:40

      run@config/spec-bundle.js:225038:49

      config/spec-bundle.js:56875:28

      config/spec-bundle.js:228476:46

      invokeTask@config/spec-bundle.js:225321:36

      runTask@config/spec-bundle.js:225088:57

      invokeTask@config/spec-bundle.js:225395:41

      invoke@config/spec-bundle.js:225384:53

      timer@config/spec-bundle.js:226936:34

I will look into it.

aptmac · 2018-05-01T15:38:44Z

Yeah sorry about that, I've fixed up that test and rebased.

Edit: errr, i rebased incorrectly for the work-item-widget.html

Edit2: should be fixed and all good now .. my git-fu is lacking.

… and add-codebases fixes openshiftio/openshift.io#3021

fabric8cd · 2018-05-01T16:13:15Z

@aptmac fabric8/fabric8-ui:SNAPSHOT-PR-2794-8 fabric8-ui is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2794-fabric8-ui.badger.fabric8.io

jiekang · 2018-05-01T18:34:45Z

@joshuawilson Any thoughts on merging this? It should be safe to go in.

dlabrecq · 2018-05-02T14:33:41Z

Considering @michaelkleinhenz 's comments in openshiftio/openshift.io#3383 (comment), do we need these changes? That is, considering that the "add work item" flow will not work anymore, the changes may now be unnecessary after we remove the feature?

jiekang · 2018-05-02T14:39:34Z

I think the changes to the header implementation and the codebase action could still be useful. This makes sure the tabs have the correct selection graphic. It's fixing a much smaller issue though and I'd say, not a P0.

jiekang · 2018-05-02T14:44:50Z

It's probably best to close this PR and work on another smaller issue/PR for the add-codebase/header work.

dlabrecq · 2018-05-02T14:45:37Z

I'm all for a new PR. I'm still concerned that the app-routing.module changes hinder us from using outlets in the future. I didn't find any navigation issues with the existing codebase flow, so just want to make certain before introducing such a major change.

jiekang · 2018-05-02T14:52:52Z

Yeah that makes sense. I think it's something we can investigate more before continuing. As for the outlets my understanding is that the main issue hindering us was how we've named

Analyze as /
Planner as /planner
Create as /create

If Analyze was /analyze this wouldn't have been a problem. I honestly think it makes more sense with /analyze instead of /, but it's too big of a change to make at this time.

There isn't a navigation issue. It's just that when using the add codebase action, it directs you to the create/codebases page. If you then navigate to create/pipelines or create/deployments, the header tab still highlights Codebases, instead of the page you are actually on. Very minor but from a user experience it could look quite bad. How could they get something so simple wrong kind of thing.

aptmac · 2018-05-02T16:09:49Z

I'm going to close this PR. The issues with the add-work-items flow will be fixed by removing the functionality completely [0] as discussed in [1].

I've noticed that the old add-codebases slider is only available with feature-flags set to production features, and this can still lead to a 404 when trying to navigate to /create for example from /${space}/(action:add-codebase). Additionally, having the add-codebases slider open on the codebases page and navigating to /create/pipelines still results in incorrect header highlighting. These can be addressed in a follow-up issue/PR. In feature-opt-in level beta and above, the slider is replaced with an overlay (and don't use the action) and so these issues don't occur.

[0] #2855
[1] openshiftio/openshift.io#3383

aptmac requested review from dlabrecq and joshuawilson April 25, 2018 14:31

aptmac force-pushed the remove-or-tweak-outlets branch from c213ee1 to 02e24c8 Compare April 27, 2018 13:53

aptmac force-pushed the remove-or-tweak-outlets branch 3 times, most recently from 54a0b48 to 3a09ca1 Compare April 27, 2018 20:06

dlabrecq suggested changes Apr 28, 2018

View reviewed changes

jiekang mentioned this pull request Apr 30, 2018

Planner: Add work items widget uses API that doesn't exist anymore openshiftio/openshift.io#3383

Closed

joshuawilson mentioned this pull request May 1, 2018

feat(launcher): updates legacy import codebase with new launcher flow #2839

Merged

dlabrecq approved these changes May 1, 2018

View reviewed changes

jiekang suggested changes May 1, 2018

View reviewed changes

aptmac force-pushed the remove-or-tweak-outlets branch from 278b070 to 16213b6 Compare May 1, 2018 15:56

fix(routing): prevent side-effects of action outlet for add-work-item…

4d75538

… and add-codebases fixes openshiftio/openshift.io#3021

aptmac force-pushed the remove-or-tweak-outlets branch from 16213b6 to 4d75538 Compare May 1, 2018 16:09

jiekang approved these changes May 1, 2018

View reviewed changes

aptmac closed this May 2, 2018

aptmac mentioned this pull request May 3, 2018

fix(add-codebase): fix header highlighting when navigating pages with action, and remove add-codebase button from codebases widget #2865

Merged

aptmac deleted the remove-or-tweak-outlets branch May 15, 2018 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(routing): prevent side-effects of action outlet for add-work-item and add-codebases #2794

fix(routing): prevent side-effects of action outlet for add-work-item and add-codebases #2794

aptmac commented Apr 24, 2018 •

edited

Loading

fabric8cd commented Apr 27, 2018

dlabrecq Apr 28, 2018

dlabrecq Apr 28, 2018

jiekang Apr 30, 2018 •

edited

Loading

joshuawilson Apr 30, 2018

catrobson Apr 30, 2018

dlabrecq May 1, 2018 •

edited

Loading

jiekang May 1, 2018

dlabrecq May 1, 2018 •

edited

Loading

jiekang May 1, 2018

dlabrecq Apr 28, 2018

aptmac Apr 29, 2018

dlabrecq Apr 28, 2018

aptmac Apr 29, 2018

jiekang commented Apr 30, 2018 •

edited

Loading

jiekang commented Apr 30, 2018 •

edited

Loading

jiekang commented Apr 30, 2018 •

edited

Loading

jiekang commented Apr 30, 2018

joshuawilson commented Apr 30, 2018

dlabrecq commented Apr 30, 2018 •

edited

Loading

dlabrecq commented May 1, 2018 •

edited

Loading

jiekang commented May 1, 2018

jiekang left a comment •

edited

Loading

aptmac commented May 1, 2018 •

edited

Loading

fabric8cd commented May 1, 2018

jiekang commented May 1, 2018

dlabrecq commented May 2, 2018 •

edited

Loading

jiekang commented May 2, 2018

jiekang commented May 2, 2018

dlabrecq commented May 2, 2018

jiekang commented May 2, 2018

aptmac commented May 2, 2018

fix(routing): prevent side-effects of action outlet for add-work-item and add-codebases #2794

fix(routing): prevent side-effects of action outlet for add-work-item and add-codebases #2794

Conversation

aptmac commented Apr 24, 2018 • edited Loading

fabric8cd commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiekang Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlabrecq May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlabrecq May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiekang commented Apr 30, 2018 • edited Loading

jiekang commented Apr 30, 2018 • edited Loading

jiekang commented Apr 30, 2018 • edited Loading

jiekang commented Apr 30, 2018

joshuawilson commented Apr 30, 2018

dlabrecq commented Apr 30, 2018 • edited Loading

dlabrecq commented May 1, 2018 • edited Loading

jiekang commented May 1, 2018

jiekang left a comment • edited Loading

Choose a reason for hiding this comment

aptmac commented May 1, 2018 • edited Loading

fabric8cd commented May 1, 2018

jiekang commented May 1, 2018

dlabrecq commented May 2, 2018 • edited Loading

jiekang commented May 2, 2018

jiekang commented May 2, 2018

dlabrecq commented May 2, 2018

jiekang commented May 2, 2018

aptmac commented May 2, 2018

aptmac commented Apr 24, 2018 •

edited

Loading

jiekang Apr 30, 2018 •

edited

Loading

dlabrecq May 1, 2018 •

edited

Loading

dlabrecq May 1, 2018 •

edited

Loading

jiekang commented Apr 30, 2018 •

edited

Loading

jiekang commented Apr 30, 2018 •

edited

Loading

jiekang commented Apr 30, 2018 •

edited

Loading

dlabrecq commented Apr 30, 2018 •

edited

Loading

dlabrecq commented May 1, 2018 •

edited

Loading

jiekang left a comment •

edited

Loading

aptmac commented May 1, 2018 •

edited

Loading

dlabrecq commented May 2, 2018 •

edited

Loading