Skip to content
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.

Sign up for GitHub

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

Jump to bottom

InDesign "Dynamic Deck Dynamo" #2266

Merged
merged 37 commits into from
Apr 30, 2020
Merged

InDesign "Dynamic Deck Dynamo" #2266

merged 37 commits into from
Apr 30, 2020

Conversation

jasiwal
Copy link
Contributor

@jasiwal jasiwal commented Apr 15, 2020 •
edited
Loading

A Dynamic Deck Dynamo is a tool that uses InDesign’s XML import functionality to insert data and images from AEM into a generated InDesign document.

Prerequisite: We need InDesign Server running with AEM instance to use this tool.

More about DDD Thanks to Peter Nam for presentation.

Thanks to John Bennett for Image below

image

@update-changelog
Copy link

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the CHANGELOG.md file with this pull request.

@jasiwal
Copy link
Contributor Author

jasiwal commented Apr 15, 2020

I am working on codeclimate issues.

davidjgonzalez
davidjgonzalez requested changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@davidjgonzalez davidjgonzalez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! just a handful of minor items!

bundle/pom.xml Outdated Show resolved Hide resolved
...a/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/DynamicDeckInitiatorPageModel.java Outdated Show resolved Hide resolved
...a/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/DynamicDeckInitiatorPageModel.java Outdated Show resolved Hide resolved
...a/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/DynamicDeckInitiatorPageModel.java Outdated Show resolved Hide resolved
...a/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/DynamicDeckInitiatorPageModel.java Outdated Show resolved Hide resolved
...s/commons/indesign/dynamicdeckdynamo/osgiconfigurations/DynamicDeckConfigurationService.java Outdated Show resolved Hide resolved
...a/com/adobe/acs/commons/indesign/dynamicdeckdynamo/services/impl/DynamicDeckServiceImpl.java Outdated Show resolved Hide resolved
...a/com/adobe/acs/commons/indesign/dynamicdeckdynamo/services/impl/DynamicDeckServiceImpl.java Show resolved Hide resolved
...java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/servlets/TriggerDeckDynamoServlet.java Outdated Show resolved Hide resolved
...le/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/servlets/package-info.java Outdated Show resolved Hide resolved
joerghoh
joerghoh reviewed Apr 15, 2020
View reviewed changes
...java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/GenericListSelectComponent.java
return Collections.emptyMap();
}
Optional<String> listPath = getOption(GENERIC_LIST_PATH);
Page genericListPage = pageManager.getPage(listPath.get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are already working with Optional you could use .map(...) to make the subsequent steps much more readable.

joerghoh
joerghoh reviewed Apr 15, 2020
View reviewed changes
...java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/GenericListSelectComponent.java
@@ -0,0 +1,74 @@
package com.adobe.acs.commons.indesign.dynamicdeckdynamo.models;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this model class exported (and can be referenced from other bundles)?

joerghoh
joerghoh reviewed Apr 15, 2020
View reviewed changes
...java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/servlets/TriggerDeckDynamoServlet.java Outdated
} catch (DynamicDeckDynamoException e) {
response.setStatus(500);
jsonResponse.addProperty(MESSAGE, "Deck creation Failed!");
LOGGER.error("Exception occurred while initiating the deck generation", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe logging some more parameters could help to diagnose the error better.

@davidjgonzalez
Copy link
Contributor

@jasiwal I made a small organizational adjustments (reducing the public API space, adding Providertypes) here: https://github.com/davidjgonzalez/acs-aem-commons/tree/feature/dynamic-deck-dymano-prod

But i can't seem to PR into your repo/branch (IIRC you have to allow this on your repo/profile?)

@davidjgonzalez
Copy link
Contributor

@jasiwal also - I noticed that the build for this is failing due to the DialogResourceProviderFactoryTest (can't call activate).. it doesn't look like you touched anything that would cause this test to behave differently..? Am i missing something?

@jasiwal
Copy link
Contributor Author

jasiwal commented Apr 29, 2020

@davidjgonzalez Yes, I have not touched anything in "test" area. I am not sure why test is failing. I am just using MCP @formfield for constructing the Dynamic Deck form.

@davidjgonzalez
Copy link
Contributor

@badvision The test for DialogResourceProviderFactoryTest is failing on this branch, however, I don't see anything in this PR that would affect that test (or cause it to fail in the way it's failing).
Any idea what's going on w/ this?

@jasiwal
Copy link
Contributor Author

jasiwal commented Apr 29, 2020

@badvision Just to add, I am using GeneratedDialog just to create a form page in below file.

https://github.com/Adobe-Consulting-Services/acs-aem-commons/blob/31b79673901437b6a23bd375bd6e02f328deac0b/bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/DynamicDeckInitiatorPageModel.java

@badvision
Copy link
Contributor

By the sound of it, the dialog provider OSGi service isn't able to start because some dependent OSGi service isn't injected first. I would need to know more about the stacktrace to understand the issue fully. The main branch is working, FWIW.

@davidjgonzalez
Copy link
Contributor

@badvision can your DialogResourceProviderFactoryTest be effected by OTHER tests being run? That sounds weird/bad? IIRC it does some funky class-loading stuff, could that straddle the Test Suite barrier?

@davidjgonzalez
Copy link
Contributor

@badvision here's the error from travis..

[ERROR] Tests run: 3, Failures: 0, Errors: 3, Skipped: 0, Time elapsed: 0.172 s <<< FAILURE! - in com.adobe.acs.commons.mcp.DialogResourceProviderFactoryTest
[ERROR] testResourceResolution(com.adobe.acs.commons.mcp.DialogResourceProviderFactoryTest)  Time elapsed: 0.119 s  <<< ERROR!
java.lang.RuntimeException: Unable to invoke method 'activate' for class com.adobe.acs.commons.mcp.impl.DialogResourceProviderFactoryImpl
	at com.adobe.acs.commons.mcp.DialogResourceProviderFactoryTest.init(DialogResourceProviderFactoryTest.java:49)
Caused by: java.lang.NullPointerException
	at com.adobe.acs.commons.mcp.DialogResourceProviderFactoryTest.init(DialogResourceProviderFactoryTest.java:49)

@davidjgonzalez
Copy link
Contributor

davidjgonzalez commented Apr 30, 2020 •
edited
Loading

Fixed the issue w the failing tests here: https://github.com/jasiwal/acs-aem-commons/pull/2/files

Didn't look too deep, but the inherited getHelper() method was returning null for some reason. Also, the DialogResourceProviderFactoryTest pulls registers 40+ classes in its activate, including the DynamicDeckDynamoPageModel from this PR, which was causing things to break in DialogResourceProviderFactoryTest

@badvision it's probably worth looking at how this can be isolated. Pretty confusing when something that simply uses your MCP Forms breaks their (MCP Forms) tests...

ADOBENET\jasiwal added 2 commits April 30, 2020 08:07
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5661

  • 15 of 666 (2.25%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.3%) to 54.838%
Changes Missing Coverage Covered Lines Changed/Added Lines %
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/services/impl/DynamicDeckConfigurationServiceImpl.java 0 5 0.0%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/exception/DynamicDeckDynamoException.java 0 8 0.0%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/utils/XMLResourceIterator.java 0 10 0.0%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/CollectionSelectComponent.java 6 18 33.33%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/DynamicDeckInitiatorPageModel.java 3 21 14.29%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/models/GenericListSelectComponent.java 6 30 20.0%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/servlets/impl/TriggerDeckDynamoServlet.java 0 46 0.0%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/services/impl/DynamicDeckServiceImpl.java 0 99 0.0%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/utils/DynamicDeckUtils.java 0 122 0.0%
bundle/src/main/java/com/adobe/acs/commons/indesign/dynamicdeckdynamo/workflow/processes/impl/DynamicDeckBackTrackProcess.java 0 148 0.0%
Totals Coverage Status
Change from base Build 5656: -1.3%
Covered Lines: 15370
Relevant Lines: 28028
💛 - Coveralls

@badvision
Copy link
Contributor

So the reason it does that is because it looks at every model to see if that model is a dialog resource provider or not. I could change that annotation to instead be a compile-time annotation and add an annotation processor to a model which puts the dialog generation gunk in there. This would obviate the need for the dialog provider service I think.

@badvision
Copy link
Contributor

Any way we can improve test coverage before calling this complete?

@davidjgonzalez
Copy link
Contributor

@badvision Just a FYI - we're planning on merging this for initial release with test coverage coming behind it, due to some urgent project need for this feature. This feature also comes from a real-life project so its been battle-hardened already (though this, of course, does not reduce the need for tests, but is a welcome bonus!)

@badvision
Copy link
Contributor

Well if you're okay with it then I'm ok with it but can we first create an issue to improve testing so we track that work too? :)

@davidjgonzalez
Copy link
Contributor

@badvision sure: #2279

@davidjgonzalez davidjgonzalez merged commit c11d590 into Adobe-Consulting-Services:master Apr 30, 2020
@stefanseifert stefanseifert mentioned this pull request May 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidjgonzalez davidjgonzalez davidjgonzalez requested changes

@joerghoh joerghoh joerghoh left review comments

@justinedelson justinedelson justinedelson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasiwal @davidjgonzalez @badvision @coveralls @justinedelson @joerghoh