Closes #1075 - Implement a wizard dialog for creating scopes definitions in the graphical configuration view #1092

Patrick-Eichhorn · 2021-05-27T13:12:56Z

This change is

mariusoe

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @Patrick-Eichhorn)

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 1 at r1 (raw file):

import { Fieldset } from 'primereact/fieldset';

Regarding the directory: Please use the following casing for the directory name scope-wizard

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 57 at r1 (raw file):

Class Matcher

Let's rename this to type matcher because we use the term type in the yaml as well and we can also match on interfaces which are no classes.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 58 at r1 (raw file):

row-center row-margin meta-row

You can merge these classes into a single one because they are only used here.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 64 at r1 (raw file):

            options={classMatchers}
            onChange={(e) => setState('currentClassMatcher', e.value)}
            placeholder="Select a Class Matcher"

Let's discuss this: how about setting the matcher to Class and the mode to equals by default? I would expect that users notice the dropdown, thus, the additonal options to chose from. In addition, we could make the dropdown boxes smaller, so the input field for the pattern can be made wider.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 67 at r1 (raw file):

which

How about adding name here as well. So the text would be (Class/Superclass/interface) which name (equals/starts with/...) ...

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 81 at r1 (raw file):

Class or Interface Name

How about Name Pattern?

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 89 at r1 (raw file):

onClick={() => alert('Todo: Open Class Browser')} //showClassBrowserDialog()

Let's also add a callback for this. The dialog shoud handle the interaction with the class browser and not the class matcher.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.stories.js, line 14 at r1 (raw file):

export const Default = Template.bind({});
Default.args = {
  classMatcher: { currentClassMatcher: '', classMatcherType: '', className: '' },

Can you add a story with prefilled elements?

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/MethodMatcher.js, line 145 at r1 (raw file):

Method which

Method which name sound better to me and indicates that it is related to its name.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/MethodMatcher.js, line 174 at r1 (raw file):

          <label htmlFor={'onlyWithSelectedParameters'}>Only with specified arguments:</label>
        </div>
        <div className="parameterInputFields">

Merge this and the next div element into a single one because this only wraps the next one, so there is no benefit of using multiple ones.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/MethodMatcher.js, line 175 at r1 (raw file):

style={{ paddingTop: 0, overflowY: 'auto', height: '15rem' }}

Please use classes instead of inline styles, so the code is consistent in this.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/MethodMatcher.js, line 175 at r1 (raw file):

        </div>
        <div className="parameterInputFields">
          <div style={{ paddingTop: 0, overflowY: 'auto', height: '15rem' }}>

Instead of setting the height to a fixed value, it would be better to use a column-oriented flexbox for this container, so the children elements implicitly defines its height. The arguments matcher can fill the remaining space, so it grows in case the dialog is bigger, in addion, you can specify a min-height which it should use.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/MethodMatcher.js, line 207 at r1 (raw file):

Argument

How about Argument Class or Argument Full-Qualified Class Name?

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardConstants.js, line 1 at r1 (raw file):

export const classMatchers = [

Constants in upper case please.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardConstants.js, line 15 at r1 (raw file):

  { label: 'contains (case-insensitive)', value: 'CONTAINS_IGNORE_CASE' },
  { label: 'ends with', value: 'ENDS_WITH' },
  { label: 'ends with (case-insensitive)', value: 'ENDS_WITH_IGNORE_CASE' },

Let's use a consistent naming for these constants. Here you use label and value. In the methodVisibility you use name and key. Let's use a common naming schema.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardConstants.js, line 19 at r1 (raw file):

export const methodVisibility = [
  { name: 'public', key: 'public' },

Use upper casing for the keys, so it is equal to the yaml configuration PUBLIC, PRIVATE, ...

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardDialog.js, line 16 at r1 (raw file):

currentClassMatcher: '', classMatcherType: ''

Should we use default values for this? See the comment in the class matcher.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardDialog.js, line 18 at r1 (raw file):

selectedMethodVisibilities: _.clone(methodVisibility),

You fill this element with objects like { name: 'private', key: 'private' }

Wouldn't it be better to have a simpler structure here, like an array ["public", "private", ...]? Makes it less complicated and easier to process afterwards.

You could do something like: _.map(methodMatcher.selectedMethodVisibilities, (e) => e.key)

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardDialog.js, line 18 at r1 (raw file):

selectedMethodVisibilities

Just visibilities. We know that this is related to methods and that these are the selected ones.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardDialog.js, line 19 at r1 (raw file):

methodMatcherType

matcherType

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardDialog.js, line 24 at r1 (raw file):

methodName

name

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardDialog.js, line 30 at r1 (raw file):

  const footer = (
    <div>
      <Button label="Apply" onClick={() => onScopeWizardApply(classMatcher, methodMatcher)} />

Disable this button in case the configuration is not "complete". E.g. missing name patterns in the class or method matcher.

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ScopeWizardDialog.js, line 113 at r1 (raw file):

onScopeWizardApply

Just onApply - we know that it is the scope wizard :)

Patrick-Eichhorn

Reviewable status: 1 of 10 files reviewed, 23 unresolved discussions (waiting on @mariusoe)

components/inspectit-ocelot-configurationserver-ui/src/components/views/configuration/scopeWizard/ClassMatcher.js, line 1 at r1 (raw file):