feat: Add useTopics model hook and query for topics listing #136

ajborley · 2020-12-02T16:47:09Z

This commit adds a new useTopics model hook, which makes GraphQL
requests to the server to administer Kafka topics.
The hook returns a set of sub-hooks, with just the useGetTopics
sub-hook implemented for now. useGetTopics returns the list of topics
(including name, number of partitions and number of replicas).

Contributes to: #124

Signed-off-by: Andrew Borley [email protected]

github-actions · 2020-12-02T16:51:13Z

PR Report

Bundle Sizes

View bundle sizes for 'client'

Bundle	New Size	Original Size	Increase/Decrease	File
1.bundle.js	633.3KB	631.33KB	0%	1.bundle.js
Strimzi UI JS Bundle	12.09KB	11.08KB	9%	main.bundle.js

Overall bundle size change: 0.47%

View bundle sizes for 'server'

Bundle	New Size	Original Size	Increase/Decrease	File
Strimzi UI Server JS Bundle	26.71KB	26.67KB	0%	main.js

Overall bundle size change: 0.16%

Test Coverage

View test coverage

File	Lines	Statement	Functions	Branches
Total	100%	100%	100%	100%
client/Bootstrap/Navigation/useRouteConfig/useRouteConfig.hook.ts	100%	100%	100%	100%
client/Contexts/ConfigFeatureFlag/Context.tsx	100%	100%	100%	100%
client/Contexts/ConfigFeatureFlag/FeatureFlag.view.tsx	100%	100%	100%	100%
client/Contexts/Introspect/Introspection.ts	100%	100%	100%	100%
client/Contexts/Logging/Context.tsx	100%	100%	100%	100%
client/Hooks/useConfigFeatureFlag/useConfigFeatureFlag.ts	100%	100%	100%	100%
client/Hooks/useLogger/Hook.ts	100%	100%	100%	100%
client/Panels/Home/Home.tsx	100%	100%	100%	100%
client/Panels/Topics/Model.ts	100%	100%	100%	100%
client/Panels/Topics/View.carbon.tsx	100%	100%	100%	100%
client/Utils/sanitise/sanitise.ts	100%	100%	100%	100%
client/Utils/window/window.ts	100%	100%	100%	100%

File	Lines	Statement	Functions	Branches
Total	100%	100%	100%	100%
server/api/controller.ts	100%	100%	100%	100%
server/api/router.ts	100%	100%	100%	100%
server/client/controller.ts	100%	100%	100%	100%
server/client/router.ts	100%	100%	100%	100%
server/config/controller.ts	100%	100%	100%	100%
server/config/router.ts	100%	100%	100%	100%
server/core/app.ts	100%	100%	100%	100%
server/core/modules.ts	100%	100%	100%	100%
server/log/router.ts	100%	100%	100%	100%
server/mockapi/data.ts	100%	100%	100%	100%
server/mockapi/router.ts	100%	100%	100%	100%

Triggered by commit: a4c0f81

matthew-chirgwin

Thanks for this Andy!

The core code looks good - as per the doc however, the model hook should be private, and defined in the 'Topics' panel component (along with tests to match/align). I also think a few capabilities may be need adding to the hook too.

Other than that, this LGTM

matthew-chirgwin · 2020-12-03T10:50:11Z

client/Models/README.md

@@ -0,0 +1,37 @@
+# Models


The model for a Panel/Group is a hook, but really should be a private one - ie useTopics (I would suggest a rename to useTopicModel to be hyper picky) would be defined in a Model.ts file in the Topics Panel (and not exported as it is private/internals).

As for testing, Panels are meant to be behaviourally tested. Ie, a table with data appears with the expected info. As there is no UI (yet), thus no table, it makes asserting (indirectly) that the model is behaving abit more obtuse. I think what would be sensible is to add a placeholder View.patternfly.ts/View.carbon.ts, which just stringifies the model output. Then, tests can assert the right data is present (just not in a table), which can then be updated when the component work comes along

Just to confirm - models live inside the component that needs them, whereas reusable hooks go in the hooks dir?

Therefore, useTopics is a particular hook (if we want to encapsulate for reuse) - and the Model can either call useTopics or just the default useQuery?

Just to confirm - models live inside the component that needs them, whereas reusable hooks go in the hooks dir?

Therefore, useTopics is a particular hook (if we want to encapsulate for reuse) - and the Model can either call useTopics or just the default useQuery?

Yes - absolutley. If the intent was to have a reusable hook representing/abstracting the query, than that should be in Hooks. Given the context (and other changes), I reviewed/interpreted it as a model hook - and that (as is), model hooks are defined with their components, not in their own section (so I am expecting the code to move, and this readme to be removed).

Rearranged/refactored the code into a private hook in the Topics panel component.

matthew-chirgwin · 2020-12-03T10:51:28Z

client/Models/useTopics/Hook.ts

+import { useTopicsType } from './useTopics.types';
+
+export const useTopics = (): useTopicsType => {
+  const useGetTopics = () => useQuery(GET_TOPICS);


I would cross reference the behviours described here: #70 (comment)

I think we will need to also have a variable for topic name, which gets debounced etc

Should also add, the consumer of the model is the view. Within reason, if there are things the model can do to make the view as simple as possible (eg if no data returning an empty array), or obfiscate (loading, error states etc) that would be good too.

Have added client-side filter handling for the topics listing. The mockapi server doesn't support this yet, so it currently doesn't make any difference when running the webpack-dev-server, but you can see the filter input field changes are debounced correctly by watching the network calls..

nictownsend · 2020-12-03T16:05:41Z

client/Models/README.md

+
+## Test approach
+
+Elements should be tested in a functional manor. See [Test Driven Development](../../docs/Test.md#style-of-test).


Suggested change

Elements should be tested in a functional manor. See [Test Driven Development](../../docs/Test.md#style-of-test).

Elements should be tested in a functional manner. See [Test Driven Development](../../docs/Test.md#style-of-test).

Fixed - along with all the others :)

nictownsend · 2020-12-03T16:11:25Z

client/Panels/Home/Home.tsx

  return (
    <div className='home'>
      <img src={image} alt='Strimzi logo' />
      <p>Welcome to the Strimzi UI</p>
      {showVersion && isComplete && <p>{`Version: ${version}`}</p>}
+      {loading || !data ? (
+        <p>Loading...</p>


Translation ;)

nictownsend · 2020-12-03T16:14:42Z

client/Models/README.md

@@ -0,0 +1,37 @@
+# Models


Just to confirm - models live inside the component that needs them, whereas reusable hooks go in the hooks dir?

Therefore, useTopics is a particular hook (if we want to encapsulate for reuse) - and the Model can either call useTopics or just the default useQuery?

- This commit adds a new `useTopics` model hook, which makes GraphQL requests to the server to administer Kafka topics. - The hook returns a set of sub-hooks, with just the `useGetTopics` sub-hook implemented for now. `useGetTopics` returns the list of topics (including name, number of partitions and number of replicas). Contributes to: strimzi#124 Signed-off-by: Andrew Borley <[email protected]>

- Addresses initial review comments. Contributes to: strimzi#124 Signed-off-by: Andrew Borley <[email protected]>

matthew-chirgwin

Couple of minor comments

matthew-chirgwin · 2020-12-07T10:19:30Z

client/Panels/Home/Home.steps.tsx

@@ -52,7 +52,7 @@ When('it is rendered with no version', () => {
  showVersionSet = false;
 });

-Then('it should display the expected text', () => {
+Then('it should display the expected text', async () => {


Wont block the PR on it, but was this intended?

matthew-chirgwin · 2020-12-07T10:23:26Z

client/Panels/Topics/Topics.steps.tsx

+  fireEvent.change(renderResult.getByPlaceholderText('filter'), {
+    target: { value: filter },
+  });
+  await waitFor(() => {


My personal preference would be that rather than using mock timers, we use the apollo helpers, eg

strimzi-ui/client/Contexts/ConfigFeatureFlag/ConfigFeatureFlag.spec.tsx

Line 290 in a4c0f81

await apolloMockResponse(); // tick for data

matthew-chirgwin · 2020-12-07T10:24:25Z

client/Panels/Topics/useTopicsModel.assets.ts

+ * Copyright Strimzi authors.
+ * License: Apache License 2.0 (see the file LICENSE or http://apache.org/licenses/LICENSE-2.0.html).
+ */
+import { generateMockDataResponseForGQLRequest } from 'utils/test/withApollo/withApollo.util';


import { generateMockDataResponseForGQLRequest } from 'utils/test' ?

matthew-chirgwin · 2020-12-07T10:25:08Z

client/Panels/Topics/useTopicsModel.spec.tsx

+describe('`useTopics` hook', () => {
+  describe('`useGetTopics` hook', () => {
+    beforeEach(() => {
+      jest.useFakeTimers();


Same comment here re use of helper functions, and not to control timers

matthew-chirgwin · 2020-12-07T10:31:18Z

client/Panels/Topics/Model.ts

+  };
+
+  return {
+    useGetTopics,


This is interesting (and not what I expected - not to say that is wrong!). I expected that we would also be owning/exporting the state for the filter - ie its value and function to update it, which in fact is a (lodash) debounced version. When that debounce clears, then the value is set, firing the query. Does this mean the view is making use of a useState call to track the filter value?

Edit: having seen the code below, I see this is the case. My preference would be the Model does all hook work, and that the view imports as few hooks as possible (for easy sharing of the business logic/keeping the view layers thin)

@matthew-chirgwin

- Based on review comments from @matthew-chirgwin Contributes to: strimzi#124 Signed-off-by: Andrew Borley <[email protected]>

ajborley · 2020-12-07T21:24:32Z

@matthew-chirgwin - I have pushed up a new commit based on your suggestions. It seems to work fine in webpack-dev-server and the e2e server, and tests are passing, but I'm getting an eslint warning that I haven't been able to get past so far.

pmuir · 2020-12-09T19:56:29Z

client/Bootstrap/index.tsx


 init(); //Bootstrap i18next support
 ReactDOM.render(
  <ApolloProvider client={apolloClient}>
    <ConfigFeatureFlagProvider>
      <LoggingProvider>
        <FeatureFlag flag={'client.Pages.PlaceholderHome'}>
-          <Home />
+          <Home>
+            <Topics />


Needs a router, not hard coding like this.

pmuir · 2020-12-09T19:57:15Z

client/Panels/Topics/Model.ts

+import debounce from 'lodash.debounce';
+
+const onChangeEvent = (
+  evt: ChangeEvent<HTMLInputElement>,


Don't use html events here, it breaks the MVVC abstraction. Use scalar types or add an abstraction.

pmuir · 2020-12-09T19:57:37Z

client/Panels/Topics/Model.ts

+    debounce(setTopicsFilter, 500),
+    []
+  );
+  const { data, loading, error } = useQuery<topicsType>(GET_TOPICS, {


Types start with capital letters.

pmuir · 2020-12-09T19:57:53Z

client/Panels/Topics/Topics.steps.tsx

+  fireEvent.change(renderResult.getByPlaceholderText('filter'), {
+    target: { value: filter },
+  });
+  await waitFor(() => {


pmuir · 2020-12-09T19:58:29Z

client/Panels/Topics/useTopicsModel.types.ts

+  name: string;
+  partitions: number;
+  replicas: number;
+};


Use the types generated by the Rama's PR here.

pmuir · 2020-12-09T19:58:57Z

client/Queries/Topics/index.ts

+      replicas
+    }
+  }
+`;


Not valid graphql schema - update to latest schema from admin server.

matthew-chirgwin suggested changes Dec 3, 2020

View reviewed changes

matthew-chirgwin linked an issue Dec 3, 2020 that may be closed by this pull request

Implement model & controller for Topics (listing) page #124

Closed

nictownsend reviewed Dec 3, 2020

View reviewed changes

ajborley added 2 commits December 4, 2020 21:11

fix: Refactor to Topic panel

1da8fda

- Addresses initial review comments. Contributes to: strimzi#124 Signed-off-by: Andrew Borley <[email protected]>

ajborley force-pushed the 124-useTopics-model branch from 70105af to 1da8fda Compare December 4, 2020 21:11

matthew-chirgwin suggested changes Dec 7, 2020

View reviewed changes

matthew-chirgwin requested review from a team, pmuir and dlabaj December 7, 2020 10:32

scholzj approved these changes Dec 7, 2020

View reviewed changes

pmuir approved these changes Dec 7, 2020

View reviewed changes

fix: Initial Model update

86cd532

- Based on review comments from @matthew-chirgwin Contributes to: strimzi#124 Signed-off-by: Andrew Borley <[email protected]>

pmuir reviewed Dec 9, 2020

View reviewed changes

pmuir suggested changes Dec 9, 2020

View reviewed changes

Base automatically changed from master to main March 25, 2021 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add useTopics model hook and query for topics listing #136

feat: Add useTopics model hook and query for topics listing #136

ajborley commented Dec 2, 2020

github-actions bot commented Dec 2, 2020 •

edited

Loading

Overall bundle size change: 0.47%

Overall bundle size change: 0.16%

matthew-chirgwin left a comment

matthew-chirgwin Dec 3, 2020

nictownsend Dec 3, 2020

matthew-chirgwin Dec 3, 2020 •

edited

Loading

ajborley Dec 4, 2020

matthew-chirgwin Dec 3, 2020

matthew-chirgwin Dec 3, 2020

ajborley Dec 4, 2020 •

edited

Loading

nictownsend Dec 3, 2020

ajborley Dec 4, 2020

nictownsend Dec 3, 2020

nictownsend Dec 3, 2020

matthew-chirgwin left a comment

matthew-chirgwin Dec 7, 2020

matthew-chirgwin Dec 7, 2020

pmuir Dec 9, 2020

matthew-chirgwin Dec 7, 2020

matthew-chirgwin Dec 7, 2020

matthew-chirgwin Dec 7, 2020

ajborley commented Dec 7, 2020

pmuir Dec 9, 2020

pmuir Dec 9, 2020

pmuir Dec 9, 2020

pmuir Dec 9, 2020

pmuir Dec 9, 2020

pmuir Dec 9, 2020


		## Test approach

		Elements should be tested in a functional manor. See [Test Driven Development](../../docs/Test.md#style-of-test).

feat: Add useTopics model hook and query for topics listing #136

Are you sure you want to change the base?

feat: Add useTopics model hook and query for topics listing #136

Conversation

ajborley commented Dec 2, 2020

github-actions bot commented Dec 2, 2020 • edited Loading

PR Report

Bundle Sizes

Overall bundle size change: 0.47%

Overall bundle size change: 0.16%

Test Coverage

matthew-chirgwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-chirgwin Dec 3, 2020 • 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

ajborley Dec 4, 2020 • 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

matthew-chirgwin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajborley commented Dec 7, 2020

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

github-actions bot commented Dec 2, 2020 •

edited

Loading

matthew-chirgwin Dec 3, 2020 •

edited

Loading

ajborley Dec 4, 2020 •

edited

Loading