Add Automation UI #5622

felixfbecker · 2019-09-17T00:43:26Z

Adds the UI for the automation 3.8 milestone.

New pages:

Global campaigns list
Global campaign details list
- Shows progress bar with status (no historical burndown chart yet)
- Lists changesets (no add functionality yet, need to use API)
- No delete, edit yet
Global new campaign form (with dropdown for namespace)

I am using React hooks for all components, and a new hook for Observables.

codecov · 2019-09-17T02:59:54Z

Codecov Report

Merging #5622 into master will increase coverage by 0.01%.
The diff coverage is 70.37%.

@@            Coverage Diff             @@
##           master    #5622      +/-   ##
==========================================
+ Coverage   47.57%   47.58%   +0.01%     
==========================================
  Files         757      761       +4     
  Lines       46052    46078      +26     
  Branches     2749     2752       +3     
==========================================
+ Hits        21907    21925      +18     
- Misses      22081    22089       +8     
  Partials     2064     2064

Impacted Files	Coverage Δ
web/src/routes.tsx	`17.14% <ø> (ø)`	⬆️
cmd/frontend/internal/app/jscontext/jscontext.go	`3.03% <0%> (-0.05%)`	⬇️
pkg/conf/computed.go	`24.51% <0%> (-0.82%)`	⬇️
web/src/nav/NavLinks.tsx	`81.81% <100%> (ø)`	⬆️
web/src/util/useObservable.ts	`100% <100%> (ø)`
web/src/enterprise/campaigns/icons.ts	`100% <100%> (ø)`
web/src/components/LinkWithIconOnlyTooltip.tsx	`50% <50%> (ø)`
...terprise/campaigns/global/nav/CampaignsNavItem.tsx	`50% <50%> (ø)`
... and 2 more

tsenart · 2019-09-17T09:31:00Z

@felixfbecker: Great! Are there any gaps in the backend that we need to fill still to fully integrate with this?

felixfbecker · 2019-09-17T09:49:53Z

This branch is fully functional, but uses workarounds for API deficiencies, e.g.

Query.viewerNamespaces missing
Namespace.campaigns missing

tsenart · 2019-09-17T10:55:05Z

@felixfbecker: Are those two everything? Or anything else missing?

tsenart · 2019-09-17T10:56:54Z

I couldn't find anything about Query.viewerNamespaces in RFC28. Can you help us with more details about it please?

felixfbecker · 2019-09-17T11:27:20Z

It's defined in RFC20 (RFC28 doesn't define any APIs) and also implemented in Quinn's prototype branch. It's needed to list the possible namespaces a user can pick to create a campaign under.

tsenart · 2019-09-17T12:15:56Z

RFC28 doesn't define any APIs

RFC28 is what we're working towards in this iteration, right? It certainly defines APIs, as we implement them to support the specified functionality. We've been adding you as a reviewer so that you could integrate the frontend code on an on-going basis, as well as provide valuable feedback.

What can we do going forward to get on the same page more easily?

felixfbecker · 2019-09-17T14:04:17Z

Yes, but it only talks about product/UI requirements, not GraphQL fields. I thought we were sticking to RFC20 for the fields that are needed to achieve RFC28, but it wasn't obvious which fields we need. In retrospect, I should have made a list of the subset of fields from RFC20 that are needed for RFC28. GraphQL is a client-oriented API language, and I believe it is important to design the API from the client's perspective, not from the backend's perspective with post-merge reviews (that may then be discarded because now more effort is needed to change anything, like database migrations etc). The biggest mistake was probably simply that we skipped a8n sync last week (I missed that no different time was setup after the cancellations) where we could have uncovered these misalignments.

I would propose that future API design and review for milestones is done up front in an RFC or PR (with just the schema).

tsenart · 2019-09-17T14:20:47Z

I would propose that future API design and review for milestones is done up front in an RFC or PR (with just the schema).

👍

tsenart · 2019-09-17T14:24:11Z

@felixfbecker: Can you please follow up with a PR that changes the schema to add the missing functionality in the scope of RFC20 and these frontend changes? We can discuss there next steps.

lguychard

Great stuff, I love how little boilerplate there is with hooks +useObservable() compared to how our components have traditionally been written.

I'm noticing that only useObservable() has tests. I'm assuming this is because you still expect a lot of changes in the new components in the near future? What is your planned strategy for testing these new components / new functionality?

lguychard · 2019-09-18T09:09:27Z

web/src/util/useObservable.ts

+import { useEffect, useState, useMemo } from 'react'
+import { Observable, Observer, Subject } from 'rxjs'
+
+export function useObservable<T>(observable: Observable<T>, deps: readonly unknown[]): T | undefined {


Could you add docstrings for useObservable() and useEventObservable()?

Not blocker: Also curious whether we could create a new eslint rule based on react-hooks/exhaustive-deps, that would give accurate warnings on missing useObservable() deps?

Yeah, this might be something to discuss with React to check deps parameters on custom hooks too. However, as I was writing the docblock, I realized that it is probably better to use the observable here as dep for useEffect(), and let the caller make sure it doesn't change unintentionally by using useMemo(). useMemo() will then be checked correctly (although we cannot check that useMemo() is used without a custom rule).

lguychard · 2019-09-18T09:18:51Z

web/src/enterprise/campaigns/new/NewCampaignPage.tsx

+import { LoadingSpinner } from '@sourcegraph/react-loading-spinner'
+import { CampaignsIcon } from '../icons'
+
+export const createCampaign = (input: GQL.ICreateCampaignInput): Observable<GQL.ICampaign> =>


Instinctively, I would've injected createCampaign() and other helpers calling queryGraphQL() / mutateGraphQL()) through Props so that they could be mocked in tests. Thoughts on doing that?

Yeah, I will do that once I'm adding tests (won't be difficult to refactor)

felixfbecker · 2019-09-18T09:54:06Z

I'm noticing that only useObservable() has tests. I'm assuming this is because you still expect a lot of changes in the new components in the near future? What is your planned strategy for testing these new components / new functionality?

Yes, I didn't write snapshots or e2e tests yet because they wouldn't help rn catching regressions and would hinder the refactors that will be needed to implement the next milestone of features. Right now I need to test everything manually anyway and it's small enough that I have a good handle on it. I do plan to test everything as it gets more stable.

I added tests for the hooks because I expect them to stay like this (at least the signatures) and tests gave me more confidence in them working correctly.

felixfbecker requested a review from lguychard as a code owner September 17, 2019 00:43

lguychard requested a review from a user September 17, 2019 06:50

tsenart requested a review from mrnugget September 17, 2019 09:31

tsenart added the a8n label Sep 17, 2019

felixfbecker force-pushed the automation-ui branch from c642105 to 5025fde Compare September 17, 2019 11:45

ghost approved these changes Sep 17, 2019

View reviewed changes

lguychard approved these changes Sep 18, 2019

View reviewed changes

felixfbecker added this to the 3.8 milestone Sep 18, 2019

felixfbecker added this to Needs review in Web Team :: Current iteration via automation Sep 18, 2019

felixfbecker added 9 commits September 18, 2019 14:05

Add automation UI

381d846

Display review state

2152ce2

Update snapshots

f7de612

Add tests for useObservable

1f7724c

Link to changesets on code host

3a0e9b5

Correct casing

e4107e0

Add docblocks

9175862

Simplify useEventObservable

47ccb4e

Deduplicate dependencies

a5c67be

felixfbecker force-pushed the automation-ui branch from 96e5052 to a5c67be Compare September 18, 2019 12:06

mrnugget approved these changes Sep 19, 2019

View reviewed changes

Web Team :: Current iteration automation moved this from Needs review to Reviewer approved Sep 19, 2019

felixfbecker added 2 commits September 19, 2019 13:06

Add feature flag

aaf1b8d

Remove deps arg from useObservable

242f688

felixfbecker requested a review from slimsag as a code owner September 19, 2019 11:06

felixfbecker requested review from lguychard, mrnugget and tsenart September 19, 2019 11:07

mrnugget approved these changes Sep 19, 2019

View reviewed changes

felixfbecker added 5 commits September 19, 2019 13:14

Fix missing repo URL

9c90c20

Fix NavLinks test

dc5fc21

Correct target=_blank

51e4bb0

Campaign creation fixes

72d3304

Fix ESLint

0243782

lguychard approved these changes Sep 19, 2019

View reviewed changes

felixfbecker merged commit f05ac74 into master Sep 19, 2019

Web Team :: Current iteration automation moved this from Reviewer approved to Done Sep 19, 2019

felixfbecker deleted the automation-ui branch September 19, 2019 12:02

lguychard pushed a commit that referenced this pull request Sep 19, 2019

Add Automation UI (#5622)

d6db4f7

chrispine added the batch-changes Issues related to Batch Changes label Aug 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Automation UI #5622

Add Automation UI #5622

felixfbecker commented Sep 17, 2019 •

edited

codecov bot commented Sep 17, 2019 •

edited

tsenart commented Sep 17, 2019

felixfbecker commented Sep 17, 2019

tsenart commented Sep 17, 2019

tsenart commented Sep 17, 2019

felixfbecker commented Sep 17, 2019

tsenart commented Sep 17, 2019

felixfbecker commented Sep 17, 2019

tsenart commented Sep 17, 2019

tsenart commented Sep 17, 2019

lguychard left a comment

lguychard Sep 18, 2019

felixfbecker Sep 18, 2019

lguychard Sep 18, 2019

felixfbecker Sep 18, 2019

felixfbecker commented Sep 18, 2019

Add Automation UI #5622

Add Automation UI #5622

Conversation

felixfbecker commented Sep 17, 2019 • edited

codecov bot commented Sep 17, 2019 • edited

Codecov Report

tsenart commented Sep 17, 2019

felixfbecker commented Sep 17, 2019

tsenart commented Sep 17, 2019

tsenart commented Sep 17, 2019

felixfbecker commented Sep 17, 2019

tsenart commented Sep 17, 2019

felixfbecker commented Sep 17, 2019

tsenart commented Sep 17, 2019

tsenart commented Sep 17, 2019

lguychard left a comment

Choose a reason for hiding this comment

lguychard Sep 18, 2019

Choose a reason for hiding this comment

felixfbecker Sep 18, 2019

Choose a reason for hiding this comment

lguychard Sep 18, 2019

Choose a reason for hiding this comment

felixfbecker Sep 18, 2019

Choose a reason for hiding this comment

felixfbecker commented Sep 18, 2019

felixfbecker commented Sep 17, 2019 •

edited

codecov bot commented Sep 17, 2019 •

edited