Uploaded image for project: 'Camunda Optimize'
  1. Camunda Optimize
  2. OPT-1239

I can add external sources to dashboards

    Details

    • Type: Feature Request
    • Status: Done
    • Priority: L3 - Default
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.1.0-alpha2, 2.1.0
    • Component/s: frontend
    • Labels:
      None

      Description

      AT:

      • in the dashboard edit mode, in the add a report modal, there is a text button, that, when clicked, replaces modal content with single input field and explanation text. In input field, the url of the embedded resource can be entered
      • modal title and footer stays the same
      • it is not possible to go back from the external data source view
      • when rendering an external report, we do not render a title or have any optimize specific click behavior
      • all edit mode addons apply to external addons

        Issue Links

          Activity

          Hide
          johannes.heinemann Johannes Heinemann added a comment -

          Review hints:

          • Works really nice. Well done Sebastian
          • If I paste something in the input field of the external add report modal, then I can't confirm the modal with key press enter. Could also be done in another ticket.
          • I paste random stuff in the external source input field, then I get the page itself shown instead of an error that the external source could not be loaded.
          • It is not clear for mean what the AT "when rendering an external report, we do not render a title or have any optimize specific click behavior" means, because I can still click and interact with the external resource. Even when I add Optimize content as external resource.
          • DashboardsResports.js: has two different concerns/ contains logic for different proposes. I would rather split up the class in two different classes one responsible for rendering the optimize dashboard report and one responsible for rendering the external resource.
          • DashboardsResports.js: you are adding the addons in the external rendering and the report rendering using the same code. If there are changes this is prone to error, because one might forget to apply the changes twice. let's move that code to a function.
          • AddButton.js: you could think about creating two modals - one for the external source mode and one for the add optimize report instead of using the same modal. But this is a matter of taste.
          Show
          johannes.heinemann Johannes Heinemann added a comment - Review hints: Works really nice. Well done Sebastian If I paste something in the input field of the external add report modal, then I can't confirm the modal with key press enter. Could also be done in another ticket. I paste random stuff in the external source input field, then I get the page itself shown instead of an error that the external source could not be loaded. It is not clear for mean what the AT "when rendering an external report, we do not render a title or have any optimize specific click behavior" means, because I can still click and interact with the external resource. Even when I add Optimize content as external resource. DashboardsResports.js: has two different concerns/ contains logic for different proposes. I would rather split up the class in two different classes one responsible for rendering the optimize dashboard report and one responsible for rendering the external resource. DashboardsResports.js: you are adding the addons in the external rendering and the report rendering using the same code. If there are changes this is prone to error, because one might forget to apply the changes twice. let's move that code to a function. AddButton.js: you could think about creating two modals - one for the external source mode and one for the add optimize report instead of using the same modal. But this is a matter of taste.
          Hide
          sebastian.stamm Sebastian Stamm added a comment -

          Works really nice. Well done Sebastian

          Thanks

          If I paste something in the input field of the external add report modal, then I can't confirm the modal with key press enter. Could also be done in another ticket.

          Good point. I think this behavior is also desired for some other modals where we only have a single text input field. I created OPT-1256 for it.

          I paste random stuff in the external source input field, then I get the page itself shown instead of an error that the external source could not be loaded.

          This is expected behavior. The random nonsense you pasted was probably interpreted as relative URL to the current location. When you provide an actual nonexistent URL, you will get the standard error display of the browser.

          It is not clear for mean what the AT "when rendering an external report, we do not render a title or have any optimize specific click behavior" means, because I can still click and interact with the external resource. Even when I add Optimize content as external resource.

          For normal reports we add a title that, when clicked, brings the user to the report view. This AT checks that we do not have a title for the external reports and also do not provide a way to e.g. open the external page in a new tab (apart from what may be provided by the browser). That the user is able to interact with the external source is expected.

          For the code hints: I refactored the AddButton and DashboardReport components, so that the optimize report and external report logic is in different components.

          Show
          sebastian.stamm Sebastian Stamm added a comment - Works really nice. Well done Sebastian Thanks If I paste something in the input field of the external add report modal, then I can't confirm the modal with key press enter. Could also be done in another ticket. Good point. I think this behavior is also desired for some other modals where we only have a single text input field. I created OPT-1256 for it. I paste random stuff in the external source input field, then I get the page itself shown instead of an error that the external source could not be loaded. This is expected behavior. The random nonsense you pasted was probably interpreted as relative URL to the current location. When you provide an actual nonexistent URL, you will get the standard error display of the browser. It is not clear for mean what the AT "when rendering an external report, we do not render a title or have any optimize specific click behavior" means, because I can still click and interact with the external resource. Even when I add Optimize content as external resource. For normal reports we add a title that, when clicked, brings the user to the report view. This AT checks that we do not have a title for the external reports and also do not provide a way to e.g. open the external page in a new tab (apart from what may be provided by the browser). That the user is able to interact with the external source is expected. For the code hints: I refactored the AddButton and DashboardReport components, so that the optimize report and external report logic is in different components.
          Hide
          johannes.heinemann Johannes Heinemann added a comment -

          Review hints:

          • I like the new version so much better
          • DashboardReport.test.js: I think you forgot to remove the deprecated tests. Currently, those are commented out.
          • This is expected behavior. The random nonsense you pasted was probably interpreted as relative URL to the current location. When you provide an actual nonexistent URL, you will get the standard error display of the browser.

            As discussed, we should enforce to be the url of a certain format (e.g. '(https|http)?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]+' , as a javascript regex). We should also make that transparent to the user (can be done in a separate ticket). If customers need more than the allowed formats they can still open a feature request and we can add that.

          Show
          johannes.heinemann Johannes Heinemann added a comment - Review hints: I like the new version so much better DashboardReport.test.js: I think you forgot to remove the deprecated tests. Currently, those are commented out. This is expected behavior. The random nonsense you pasted was probably interpreted as relative URL to the current location. When you provide an actual nonexistent URL, you will get the standard error display of the browser. As discussed, we should enforce to be the url of a certain format (e.g. '(https|http)?:\/\/(www\.)? [-a-zA-Z0-9@:%._\+~#=] +' , as a javascript regex). We should also make that transparent to the user (can be done in a separate ticket). If customers need more than the allowed formats they can still open a feature request and we can add that.
          Hide
          sebastian.stamm Sebastian Stamm added a comment -
          • You are right, I forgot to remove the old tests, thanks for catching it
          • I added a verification that urls need to start with http:// or https://, followed by at least one other character. We need to keep in mind that this is frontend-side validation only, so someone skilled with REST could potententially circumvent this, but imho this is not really a problem.
          Show
          sebastian.stamm Sebastian Stamm added a comment - You are right, I forgot to remove the old tests, thanks for catching it I added a verification that urls need to start with http:// or https:// , followed by at least one other character. We need to keep in mind that this is frontend-side validation only, so someone skilled with REST could potententially circumvent this, but imho this is not really a problem.

            People

            • Assignee:
              Unassigned
              Reporter:
              sebastian.stamm Sebastian Stamm
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: