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
Issue 16820 default wfactions #16833
Conversation
dotCMS/src/main/webapp/html/portlet/ext/workflows/schemes/view_steps.jsp
Show resolved
Hide resolved
dotCMS/src/main/webapp/html/portlet/ext/workflows/schemes/workflow_js.jsp
Show resolved
Hide resolved
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.
My browser is literally crashing. Let's discuss tomorrow.
final List<Contentlet> contentlets = APILocator.getContentletAPI().search | ||
(new StringBuilder("+contentType:persona +live:true +deleted:false +working:true +conhost:" + host.getIdentifier()).toString(), | ||
-1, 0, "title desc", APILocator.systemUser(), false); | ||
final Optional<Contentlet> personaOpt = contentlets.stream().findFirst(); |
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 not a PersonaDataGen to insure that we have a persona to personalize? Otherwise, all these asserts will not be run and could be useless.
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.
We should always create the data we need to run the tests and not rely on iffy data.
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.
sounds good, I will
@@ -1117,6 +1041,8 @@ public void Test_Find_Bulk_Actions_Then_Fire_Bulk_Actions_On_Custom_Content_Type | |||
final String inode = contentlet.getInode(); | |||
|
|||
try { | |||
|
|||
workflowAPI.deleteWorkflowTaskByWebAsset(contentlet.getIdentifier(), APILocator.systemUser()); |
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.
I think we need to rename this method to workflowAPI.deleteWorkflowTaskByContentletId
Also, do we have one that takes a languageId?
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.
I think I will create two method, one with the lang and another called deleteWorkflowTaskByContentletIdByAnyLanguage
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.
Done
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.
Should we call the new one destroyWorkflowForContentlet
? I am asking b/c our contentletAPI has a destroy method which does the same thing - wipes out the content in all languages.
("contentlet", "structure", | ||
Arrays.asList("structure_inode"), Arrays.asList("inode")); | ||
|
||
Assert.assertNotNull(foreignKey); |
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.
nice test 👍
import java.util.Set; | ||
|
||
public class DotDatabaseMetaDataTest extends BaseWorkflowIntegrationTest { | ||
|
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.
glad to have this test! 👍
|
||
//4) check the contentlet is on the step unpublish | ||
final WorkflowStep unpublishStep = workflowAPI.findStepByContentlet(contentlet1); | ||
Assert.assertEquals (SystemWorkflowConstants.WORKFLOW_NEW_STEP_ID, unpublishStep.getId()); |
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.
Did we skip the workflow and the listeners are moving this to the "Unpublished step"? Do we need a Thread.sleep()
in order to give the listener a chance to fire?
} | ||
|
||
@Override | ||
public Map<String, List<Map<String, Object>>> findSystemActionsMapByContentType(final List<ContentType> contentTypes) throws DotDataException { |
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.
And why is this a Map rather than a List? I'm confused :)
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.
the method is receiving a list of content types, so it is returning a Map indexed by variable with the list of mappings associated to the content type
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.
Got it! 🥇
|
||
if (UtilMethods.isSet(schemes)) { | ||
|
||
final DotConnect dotConnect = new DotConnect() |
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.
Select where in ()
type subqueries break down when there are 100s of entries. We should iterate over smaller 100 entry partitions of the list to build the larger list to return, so our dbs do not bog down.
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.
good, I will split the login in 100 to 100, thx for the advise!.
this.cache.removeSystemActionsByWorkflowAction(action.getId()); | ||
} | ||
|
||
private String createQueryIn (final Collection list) { |
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.
partition the list you are sending to the where in
clause so we don't kill the dbs.
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.
Got it, will be fixed
@@ -1088,6 +1131,33 @@ public void deleteWorkflowTask(final WorkflowTask task, final User user) | |||
+ " has been deleted by the user: " + user.getUserId()); | |||
} | |||
|
|||
@Override | |||
@WrapInTransaction | |||
public void deleteWorkflowTaskByWebAsset(final String webAsset, final User user) throws DotDataException { |
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.
Should be deleteWorkflowTaskByContentletId
. Also, do we need to respect the langauge Id? Maybe this should take the contentlet object that would include the language Id.
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.
done
@@ -144,18 +146,21 @@ public static void prepare() throws Exception { | |||
} | |||
|
|||
@AfterClass | |||
public static void cleanUpData() throws DotSecurityException, DotDataException { | |||
public static void cleanUpData() throws Exception { |
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.
We don't need to clean up data in our tests - we assume the data will be thrown out.
No description provided.