Invert LoggingAspect blacklisting to whitelisting.

Currently the LoggingAspect works by blacklisting some methods. This has caused some trouble during TSK-1709. Should we change the blacklisting to a whitelisting, so that we don’t get confused when a given JointPoint gets weaved

@Mustapha Zorgati

License header

Sebastian and I have discussed the automatic generation of license header together with our NT expert.
We have a lot of freedom and we’d love to discuss on which files to add license headers to.

@Mustapha Zorgati

Extend ProcessEngineRequester for TaskanaAdapter

It would be nice to extend the ProcessEngineRequester, in such a way that we can retrieve more informations about tasks when getting tasks. For example the name of a task.
Importance can be understood when inspecting “process_with_different_variables_in_tasks_should_result_in_taskanaTasks_with_those_variables_in_custom_attributes"-method in TestTaskAcqusition. It’s not reproducible which task we get if we call the "getTaskIdsFromProcessInstanceId"-method in the ProcessEngineRequester.

@Yakup Ensar Evli (Unlicensed)

Add a zip-method to our pro.taskana.common.internal.util.Pair class

The zip-method should combine each element of two List as Pair and return it as a List. Example:
List a = {1, 2, 3, 4}, List b = {5, 6, 7, 8} results in List c = {(1, 5), (2, 6), (3, 7), (4, 8)}.

Proposed piece of code: IntStream.range(0, a.size()).mapToObj(i -> Pair.of(a.get(i), b.get(i))).collect(Collectors.toList());

@Yakup Ensar Evli (Unlicensed)

Unclear naming for data list in TestFactory methods

We are heavily using the @TestFactory annotation in order to create a test with multiple data sets. As of now there is no rule prohibiting the use of other mechanisms of the JUnit 5 arsenal to do the same thing.
* Should we enforce the usage of @TestFactory only for this usecase?
* Should we enforce the naming of the input list? (Now we have multiple variations: valuesForTests, no input list at all, testValues, testCases, setterList, iterator …)

@Mustapha Zorgati

Rework Indices?

Currently our Indices are not complete and have to be reviewed again.
We see the following options:

• Rework Indices, remove non-basic ones

• integrate new basic indices

• Leave all Indices as is

  • Integrate any index currently used by our users

• Replace database handling by a framework (and thus the need of manually maintaining the indices)

@Elena Mokeeva (Unlicensed) / @Mustapha Zorgati

Unnecessary List

The Implementation of getting a workbasket using a List is unnecessary, because the list has maximum one element.

Open

TaskServiceImpl

James Reynaldi

@Elena Mokeeva

Rewrite ServiceLevelHandler

The logic in ServiceLevelHandler can be simplified or/and restructured. For example the usage of helper classes like DurationPrioHolder can be avoided.

Mar 4, 2022 Wird nach dem 1.4. von Mustapha und Elena angegangen und ein konkreter Vorschlag unterbreitet.

@Elena Mokeeva (Unlicensed)

@Yakup Ensar Evli (Unlicensed)

Test for sorting according to two parameters

I couldn’t find test cases for sorting according to two parameters. We should test this.

@Yakup Ensar Evli (Unlicensed)

@Elena Mokeeva (Unlicensed)

Don’t set workbasketSummary twice

In createTask method in TaskServiceImpl we set workbasketSummary twice in some cases.

@Yakup Ensar Evli (Unlicensed)

@Elena Mokeeva (Unlicensed)

Split CompleteTaskAccTest

CompleteTaskAccTest contains tests for both, completion and claiming. Should we separate the claiming tests from the completion tests?

@Elena Mokeeva (Unlicensed)

@joerg.heffner

SqlConnectionRunner Autocommit

Currently we have some logic in jobs like this:

Open

Should we put this logic more top-level? For example directly in the SqlConnectionRunner?

@joerg.heffner

@Yakup Ensar Evli (Unlicensed) @Elena Mokeeva (Unlicensed)

TaskQuery start/end region Comments

What was the reason for introducing this? Do we want to keep it?

@joerg.heffner

@Holger Hagen (Unlicensed) @Elena Mokeeva (Unlicensed)

Removed region comments

Rename GitHub repository + organization

we write TASKANA with caps. Our repository name is “taskana”, which technically is not ok. Furthermore our organization on GitHub is called “Taskana”, which is technically also not ok.

Should we rename our organization and the repository?

@Mustapha Zorgati

@Tristan Eisermann @Yakup Ensar Evli (Unlicensed)

Undefined sorting order by Attachments

Currently the sorting order by Attachment is undefined. E.g. How to handle Tasks with multiple / no Attachments. In order to create a consistent API we have to discuss how we want to handle the sorting in those 2 cases.

@Elena Mokeeva (Unlicensed)

Slack Huddles

Slack just released huddles (https://slack.com/intl/de-de/help/articles/4402059015315-Wir-stellen-vor--Slack-Team-Check-in-eine-neue-Art-Live-Audio-Besprechungen-durchzuf%C3%BChren )

I would love to use that feature to create a virtual office. That way we know who is currently active and can interact a little easier.
Let’s talk about that idea.

@Mustapha Zorgati

@Chi Nguyen (Unlicensed)

We don’t want to use Slack huddles (for now)

Why do we have a dedicated db2 SQL query

Currently the TaskQueryMapper has two implementations of queryTaskSummaries. Do we really need the dedicated implementation for db2

Rework version management

(Might be a misunderstanding from my side)

At this point, we have several versions of the some packages so it’s unclear which is used, because the version is not specified. The version of h2 was not specified, as well as hibernate.

It seems to be chaotic, maybe we can somehow structure it?

@Elena Mokeeva (Unlicensed)

@Yakup Ensar Evli (Unlicensed)

Decide on parameter names of intervals in TaskQuery, ClassificationQuery etc.

At the moment, some interval parameters are called just “intervals”, and others are called “modifiedWithin”, “createdWithin” etc. Which do we prefer?

@Elena Mokeeva (Unlicensed)

@Yakup Ensar Evli (Unlicensed)

https://taskana.atlassian.net/browse/TSK-1952

Decide on composed parameter names in Query entities

At the moment, some composed filter parameters are called with their full name (e. g. TaskQuery attachmentClassificationNameIn(String ... attachmentClassificationNames). Some other parameters have a short name, e. g. TaskQuery primaryObjectReferenceSystemInstanceNotLike(String... systemInstances);
Which do we prefer?

@Elena Mokeeva (Unlicensed)

@Mustapha Zorgati

Change as discussed on the fly as referenced above

BLOCKING RELEASE
Rename taskana-test-api module

We have a new module: taskana-test-api.

How should we name it? @Holger Hagen (Unlicensed) said that that name could be a little confusing.

@Mustapha Zorgati

Add ‘skipAuthorization’ variable to TaskQuery

Currently, TaskQuery skips authorization checks if accesssIdIn=null. That might be confusing (There was a bug regarding this implementation). Proposal: add a ‘skipAuthorization’ variable.

@Elena Mokeeva (Unlicensed)

@Mustapha Zorgati @Yakup Ensar Evli (Unlicensed)

postponed to TASKANA-201

Refactor TestPriorityServiceProvider to be simpler

Currently the TestPriorityServiceProvider.java contains a lot of business logic which is totally irrelevant for TASKANA. Can we please discuss this and maybe reduce its complexity so that the tests are more readable?

@Mustapha Zorgati

@Yakup Ensar Evli (Unlicensed)

TestPriorityServiceProvider was deleted

JavaDoc: Referencing getter/setter with @see

our JavaDoc Rules state in 7. b) that we should link our getter and setters together with @see. Currently this is not done anywhere. Therefore we want to discuss this rule.

@Elena Mokeeva (Unlicensed) / @Mustapha Zorgati

@Norman Schmidt

Rule was crossed out

JavaDoc: Enhancement for @return of getter methods

Currently the @return tag of our getter methods contains only trivial informations. Can we adjust the rules, so that we can get rid of those trivial informations?
Ideas:

• remove @return tag for getters

• use this pattern: @return <paramName>

• … (open for discussion)

@Elena Mokeeva (Unlicensed) / @Mustapha Zorgati

@Yakup Ensar Evli (Unlicensed)

timebox 30-min and try to remove @return. If not possible use @return The <paramName>.

Move Current Release Notes Page

When we create a release while some PRs are not merged yet, their current release notes have to be removed manually. This is an unnecessary step.
Suggestion: Replace the confluence page with a file inside our repository. After a release that file get’s “cleaned” automatically

@Mustapha Zorgati

@Sofie Hofmann (Unlicensed) @Chi Nguyen (Unlicensed)

We have adjusted the PR template instead.

Make database limitations visible in Java API

We have database column restrictions for our entities. Should we make them visible in the java api? An example could be the usage of the javax validation API

@Mustapha Zorgati

@Yakup Ensar Evli (Unlicensed)

Aria Label

Aria-label is an attribute on HTML elements to provide text alternatives for users who use assistive technology e.g. screen readers. Currently, it is only used in one component. Should we delete the attribute or add it wherever it is needed? I prefer to be consistent everywhere.

@Sofie Hofmann (Unlicensed)

@Chi Nguyen (Unlicensed) @Tristan Eisermann

Implement conventional commits

Currently we don’t have a convention of how to write commit messages. We could adapt “Conventional commits” specification which is used widely in projects

https://www.conventionalcommits.org/en/v1.0.0/#summary

@Chi Nguyen (Unlicensed)

@Mustapha Zorgati @Tristan Eisermann @Martin Sawilla (Unlicensed)

Naming of internal and api classes

Currently we name our variable inconsistently. Sometimes we name Impl classes with Impl and sometimes we don't.

Examples:

• taskanaEngineImpl vs taskanaEngine

• taskService vs taskServiceImpl

Should we unify this?

@Mustapha Zorgati / @Holger Hagen (Unlicensed)

@Elena Mokeeva (Unlicensed)

Exception Signature of our Controllers

Currently our REST-Controllers list every Exception they can throw explicitly. Sometimes the list of Exceptions can be multiple lines. Should we throw Exception instead of listing every single Exception like we do with our Junit tests?

@Mustapha Zorgati

@Elena Mokeeva (Unlicensed)

application-<database>.properties

Currently we have multiple application property files for each supported database.
Unfortunately only the main application property file (for H2) is maintained.

How do we want to ensure that the other property files are maintained?
I know that it is technically possible to use multiple property files in spring so that the database specific property files only contain the datasource.

A different approch is setting the datasource manually and start the database docker containers automatically.

Let’s talk about what’s reasonable and which usecases we have to consider

@Mustapha Zorgati

@Elena Mokeeva (Unlicensed) @Yakup Ensar Evli (Unlicensed)

Postponed and to be explained by @Mustapha Zorgati

Plural arguments in Queries

Currently, some arguments in Queries are in plural (ownerLongNames), and some in singular (attachmentClassificationId). Which one do we prefer?

@Elena Mokeeva (Unlicensed)

@Mustapha Zorgati @Yakup Ensar Evli (Unlicensed) @Elena Mokeeva (Unlicensed)

We agreed on using the plural form.

‘descriptionIn’ filter option for ClassificationQuery is missing

We only have the option descriptionLike for filtering using the description of a Classification. Are other filter options needed?

@Yakup Ensar Evli (Unlicensed) / @Elena Mokeeva (Unlicensed)

we agreed not to do this now

Use a helper method for creating TaskHistoryEvent in TaskServiceImpl

At the moment, we have the following very similar code snippet in TaskServiceImpl several times.

Should we create a helper method that has the constructor of the TaskHistoryEvent as method parameter?

@Elena Mokeeva (Unlicensed)

Remove AbstractWorkbasketAccessItemQuery and AbstractWorkbasketAccessItemQueryImpl

Both classes are not used anywhere outside.

@Yakup Ensar Evli (Unlicensed)

@Mustapha Zorgati

Extend HistoryEventManagerTest

We could check if there are untested scenarios involving HistoryEventManagerTest. They can be then added to HistoryEventManagerTest.

@Elena Mokeeva (Unlicensed)

Coverage is better now

Getting rid of Thread.sleep

Sonarcloud complains about Thread.sleep (as Code smell). @joerg.heffner proposed the idea of using Awaitlity ( ) for Taskana-Adapter to get rid of Thread.sleep. We should discuss if this solution is viable for Taskana too.

@Yakup Ensar Evli (Unlicensed)

@Elena Mokeeva (Unlicensed) @Norman Schmidt

if before debug-log

see comment in

@Marcel Bagemihl

Rewrite Test in TaskControllerIntTest

should_ThrowException_When_CreatingTaskWithInvalidParameter() doesn’t use RestHelper for communication. The method could probably be simplified by using RestHelper, like other methods in this test class already do.

@Elena Mokeeva (Unlicensed)