New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AF-2771: Dashbuilder Programmatic Layout API #1119
Conversation
jenkins retest this |
Hello @caponetto and @sthundat could you please review this PR? Thanks! |
jenkins execute fdb |
@@ -30,6 +31,7 @@ | |||
import org.dashbuilder.client.error.DefaultRuntimeErrorCallback; | |||
import org.dashbuilder.client.perspective.NotFoundPerspective; | |||
import org.dashbuilder.client.resources.i18n.AppConstants; | |||
import org.dashbuilder.shared.event.UpdatedRuntimeModelEvent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding unused imports? Both UpdatedRuntimeModelEvent
and Observes
<artifactId>gson</artifactId> | ||
</dependency> | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra empty line
*/ | ||
public interface DashboardValidator { | ||
|
||
static final DashboardValidator instance = new DashboardValidatorImpl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static, final and public modifiers have no use in an interface
class DashboardValidatorImpl implements DashboardValidator { | ||
|
||
private static final String MISSING_NAVIGATION_ITEM = "Navigation item %s has no corresponding page"; | ||
private static final String VALIDE_NAVIGATION = "Navigation is valid."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static final String VALIDE_NAVIGATION = "Navigation is valid."; | |
private static final String VALID_NAVIGATION = "Navigation is valid"; |
To keep the messages consistent, I would either remove the "." here or add it into other messages
} | ||
|
||
@Test | ||
public void testSucessfulValidation() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public void testSucessfulValidation() { | |
public void testSuccessfulValidation() { |
Hi William, these are the observations that I have found during my testing. Since there is a lot to test, I will continue tomorrow morning, but I can say now that "happy path" scenarios work.
Also, I have some questions:
I am sorry if these questions are too simple, but since there is not much time to test the PR, I rather ask then spend too much time figuring out myself 🙂 Also, if it will be of any help to you, this is (with some small changes now and then) the code that I was using to test the PR (if you need to reproduce something) Let me know if you need some more information on some of my points 🙂 |
Hello @barboras7 Thank you so much for your detailed review, really appreciate. Please find my comments inline
This generates the following chart Fixing it would impact all reports in Business Central, so we need to live with this glitch for now. Talking about column constraints, an improvement that could be done in a near future is validate displayer settings based on DisplayerConstraints. No validation was planned for this version, but I added a few to avoid a "trial and error" API. I could simply do this now also because it all happens on client side, after the dashboard is rendered and it changes from chart API to chart API - but it is possible to do it for the programmatic API.
Please let me know which code you used for the logo. 9.That is a great suggestion, thanks! I did changed Dashbuilder to support it - let me know if it works for you
I just forgot to allow multiple groups creation! Now when creating the navigation you can pass multiple groups. Thanks for noticing it.
Yes. the only trick at the moment is Runtime Server data set, because a new dependency will needed to be added to your project. This happens because execution server is not a CORE data set, the implementation changes from BC to Dashbuilder Runtime! The entry point for all data sets is the DataSetDefFactory class. Here's a simple Prometheus dashboard with auto refresh:
This will result in the following page: Kindly let me know your feedback - these were all great points, I really appreciate. |
jenkins execute fdb |
Hi William, thanks for your answers.
3., 4., 5. These all work as expected, thanks.
Also, I have some other observations I found today:
Please, let me know if you need more information from me to reproduce some of the behaviors I described. Thank you. |
Hello Barbora, Thanks again for your feedback.
Regarding the new items:
I have some sample dashboards in my demos repo: https://github.com/jesuino/dashbuilder-docker/tree/master/demos/dashbuilder_dev_mode/dashbuilder-dsl-dashboards/src/main/java/org/kie/dashbuilder/dashboards
Thanks and please let me know if I am missing anything! |
jenkins execute fdb |
Hello William, thanks for the fixes.
You really did a great job on this PR 🎉 |
Hello @barboras7 , Thank you so much for your great review and feedback.
Thanks again! \o/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jesuino, thanks for the PR!
I've left some comments, please check if they make sense.
@@ -51,7 +53,7 @@ | |||
void setSize(int chartWidth, int chartHeight); | |||
|
|||
void setMargin(int chartMarginTop, int chartMarginRight, int chartMarginBottom, int chartMarginLeft); | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these extra whitespaces.
@@ -0,0 +1,3 @@ | |||
function sayHello() { | |||
alert("NEVER, it is just the begging!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alert("NEVER, it is just the begging!") | |
alert("NEVER, it is just the beginning!") |
Did you mean beginning
?
@@ -0,0 +1 @@ | |||
<h1>Comp1</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is a test resource, I think we need to add the copyright header to this file.
@@ -0,0 +1,3 @@ | |||
function sayHello() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this is a test resource, I think we need to add the copyright header to this file.
//Use this to export this dashboard to run on Dashbuilder runtime | ||
// org.dashbuilder.dsl.serialization.DashboardExporter.get().export(db, | ||
// "/tmp/dashbuilder/models/Population.zip", | ||
// org.dashbuilder.dsl.serialization.DashboardExporter.ExportType.ZIP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing this commented code.
} | ||
|
||
private static DashboardZipSerializer serializerFor(ExportType type) { | ||
// only ZIP us supported at the moment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// only ZIP us supported at the moment | |
// only ZIP is supported at the moment |
Typo here.
|
||
writeNavigation(zos, dashboard.getNavigation()); | ||
|
||
writeContent(zos, DATA_SETS_BASE + "readme.md", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting "readme.md"
into a constant.
Path path = Paths.get(filePath); | ||
if (path.toFile().exists()) { | ||
try { | ||
writeContent(zos, DATA_SETS_PATH + def.getUUID() + ".csv", Files.readAllBytes(path)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting ".csv"
into a constant.
if (path.startsWith(LAYOUTS_PATH) && path.endsWith("perspective_layout")) { | ||
LayoutTemplate template = gson.fromJson(content, LayoutTemplate.class); | ||
pages.add(Page.create(template)); | ||
} | ||
|
||
if (path.startsWith(DATA_SETS_PATH) && path.endsWith("dset")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting "dset"
and "perspective_layout"
into constants.
Lines 190 and 198 would need them.
Map<String, String> entriesContent = new HashMap<>(); | ||
Path zipTemp; | ||
try { | ||
zipTemp = Files.createTempFile("dashboard", "zip"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting these hardcoded strings into constants.
Hi @jesuino, can you please take a look at the issues that SonarCloud has raised? |
Hello @caponetto I made most of the suggested changes by sonar cloud and created the PR for our BOM: kiegroup/droolsjbpm-build-bootstrap#1619 I didn't make all changes because:
Thanks! |
jenkins execute cdb |
jenkins execute fdb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, @jesuino! Thanks!
The "Security Hotspot" raised by Sonar Cloud makes sense, but I have two points:
What we did to avoid any security breach was to make the deserialize method NON PUBLIC and making it unsupported at the moment. Thanks. |
SonarCloud Quality Gate failed. |
JIRA: https://issues.redhat.com/browse/AF-2771
MERGE WITH:kiegroup/droolsjbpm-build-bootstrap#1619
This is a PR to bring a new programmatic way of creating dashboards to Dashbuilder.
Besides this, it also correct many small issues with he existing API, allows the users to export the Dashboard to a ZIP file that can run on Dashbuilder Runtime and adds a development mode to Dashbuilder Runtime!
To create programmatic dashboards first you must build
appformer/dashbuilder/dashbuilder-shared/dashbuilder-dsl
usingmvn clean install
then you can add the dependencyorg.dashbuilder:dashbuilder-dsl:7.52.0-SNAPSHOT
to your project.The API has 5 main components
org.dashbuilder.dataset.def.DataSetDefFactory
which allows you to create your own data set definition. You can also pure Java data set using the classDataSetFactory
:, for example:org.dashbuilder.dsl.factory.component.ComponentFactory
. From there you can refer to components in your local file system or build displayers components. For displayer components notice that we need a factory to build the settings, the factory is class:org.dashbuilder.displayer.DisplayerSettingsFactory
, where you can build the settings for your displayer. Bear in mind that all classes that uses a displayer will require a data set to be built, otherwise an exception will be thrown when exporting the dashboard;org.dashbuilder.dsl.factory.page.PageFactory
allow make it easy for you to create any page component;org.dashbuilder.dsl.factory.navigation.NavigationFactory
. This class can be used to define the menu of pages that will be displayed in Dashbuilder Runtime;org.dashbuilder.dsl.factory.dashboard.DashboardFactory
.Once you have your Dashboard object, you can export it to a file using the class
org.dashbuilder.dsl.serialization.DashboardExporter
.Here's very simple Dashboard using the above classes:
It will generate the following dashboard:
Now, this PR is much more than this, it adds a "dev mode" to Dashbuilder Runtime. This way while you develop and export the ZIP it will automatically update on Dashbuilder Runtime. This mode also upload automatically the list of dashboard in multi mode automatically. It is not for production, once the work is done the recommendation is go to produce with Dashbuilder using STATIC mode.
This is model list update
And dashboards can also be automatically updated:
This is can be enabled using the system property "dashbuilder.dev=true".
A sample dashboard can also be found on test DashboardZipSerializerTest.
Thanks!