Outcomes (2021)

JCI

Travis is having a lot of performance issues lately. Can we do something about this?
Should we consider switching to e.g. github actions?

@Mustapha Zorgati

We migrated to github actions.

Clean Classification interface

Classification interface has methods which are already extended by ClassificationSummary interface.

@Yakup Ensar Evli (Unlicensed)

Use Dependabot for frontend

We activated the dependabot for the frontend components. I’m not sure if we are ready for this and confident enough to integrate tickets if the build is green.

@Holger Hagen (Unlicensed)

We don’t want to update our frontend dependencies automatically since our tests might be not sufficient and the user interface might look different after a dependency change.

Processing of JSON body in REST-call

I implemented a POST REST-endpoint for the adapter outbox which consumes JSON in its body. This JSON contains a list of integers.

As of now, the JSON is directly passed to the service method and processed (read&deserialized) there. Should the controller handle the JSON processing? Or some “intermediate” class between controller and service? Or is it fine in the service business logic?

What if the body contains multiple different fields? Pass them all as parameters to the serice method after processing them in the controller?

@joerg.heffner

We decided that it’s okay to let the controller handle the JSON processing. Since this is the way it is currently implemented, there is no need for action.

InvalidArgumentException

It is not coherent how we use this Exception in cases where we don’t necessarily have to check for the passed argument. Sometimes we check the arguments explicit and throw this Exception, sometimes implicit.

Example: transferring a certain task to another workbasket; explicit→ checking taskId not be null, empty and throwing InvalidArgumentException; implicit→ getting TaskNotFound when calling getTask(…) if taskId is null, empty

@Tim Gerversmann

We want to check the ID first and throw an unchecked InvalidArgumentException which has not to be handled. In general, we also want to validate the inputs first.

Unify and simplify the update()/create() methods in the services of the core

In the services (TaskServiceImpl, …) of the core we have the methods to create or update tasks, classifications or workbaskets. Those methods are partly extremely complex and confusing as you can see in create() of TaskServiceImpl. (110 lines of code for 1 method!!!)

Open

Moreover they seem to differ unnecessarily in their structure and the way they work. Some are calling validate(), others are using initDefaultClassificationValues() or standardUpdateActions() to check and initialize the attributes of the passed item.

To improve overview and maintainability I would suggest to refactor, simplify and unify those methods.

@Tim Gerversmann

We are aware of the problem. We will postpone solving this issue.

Rest Tests

I discovered that we often use Json Strings for our REST-IntegrationTests. Personally I don’t think that they are maintainable.

Proposal: Let the Jackson ObjectMapper serialize our Entity instead of writing the Json string.

@Mustapha Zorgati

Since large Json strings are not maintainable und readable, we want to use the objectMapper to convert the Java object into the string.

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

Naming of our entities in docs and comments

I often see our entities like task, workbasket, classification etc written in two different ways wihtin java doc and/or comments in the code:

1. WITH capital starting letter: Classification

2. WITHOUT capital starting letter: classification.

Both cases can even occur in the same doc or sentence, which reads a little weird.

Should we agree on a convention concerning our entities?

@joerg.heffner

We decided that entities should start with capital letter.

Rework package structure for example modules

Idea: Move all packages from all our example modules from pro.taskana.XXX to pro.taskana.example.XXX

@Mustapha Zorgati

Task search in workplace

1. Currently, there are two ways of filtering tasks: either by workbasket or by value / type. Is there an use case, where we want to filter by workbasket AND by value / type?

2. We only display task filter options (task owner, task name, …) after filtering the tasks by workbasket or by value / type. I’d like to offer the possibility to display all filter options in the beginning (similiar to workbasket filtering). What do you think?

@Sofie Hofmann (Unlicensed)

1. We keep both search options separate.

2. We’ll offer the option to display all filter options in the beginning.

Present Surefire Reports in CI

When our CI fails, the test which caused the issue is hidden in the huge log. Sometimes the given error cannot be easily found (since sterr and stdout are not synced). This can cause confusion and finding out what broke the build can take some time.

I found a nice github action which processes the surefire reports and displays the tests which failed. I fiddled a litte around with it. Here is an example: https://github.com/mustaphazorgati/taskana/runs/1840214460?check_suite_focus=true

Let me show you what I did and what the tradeoffs are. Maybe we can implement this

@Mustapha Zorgati

We’ll use the ‘Aggregate Test Report’ step in the CI after all tests were executed to summarize all failed tests.

Yarn vs npm

Because yarn introduces “resolutions“ for frontend dependencies, which solve a problem I encountered when upgrading to Angular 11 and when working on my Thesis, I suggest to switch to yarn. Yarn also provides faster install times.

@Tristan Eisermann

We decided to use yarn since yarn introduces resolution (i.e. allows to overwrite versions of package installed by other packages). Switching to yarn is part of this PR https://github.com/Taskana/taskana/pull/1486.

JavaDocs “@link”

Currently we sometimes use the reference tag in our JavaDocs and sometimes not. To improve our API documentation it would make sense to unify it and set up rules.

@Tim Gerversmann

Test breaks if executed after 22:00

ServiceLevelPriorityAccTest#should_VerifyThatCreateAndPlannedAreClose breaks when executed after 22:00. This is not a timezone issue (like always), but more use case issue. This test has no business value after that time. Can we find a quick fix for this issue so that maniacs like me can work whenever they want?

@Mustapha Zorgati

fixed: https://taskana.atlassian.net/browse/TSK-1571

Default methods in Interfaces

It is possible to define Default Methods on Interfaces. Do we want to use that mechanic e.g. for overloaded interface methods? This might increase the readability.

@Mustapha Zorgati

In case of overlapping methods, we want to use the Default methods. We’ll make these changes incrementally.

our group ids

I just discovered that we have a different group id for our history.

Our ususal groupId: pro.taskana
for history: pro.taskana.history

I see three options:

• keep it as it is right now

• unify all groupIds to “pro.taskana” (our default)

• segregate our groupIds to e.g the main folders. This would result in pro.taskana.lib, pro.taskana.common, pro.taskana.rest (or somehow differently)

For idea 1 and 3 we have to take account how that changes our currently existing artifacts in maven central

@Mustapha Zorgati

Automate code formatting

Sometimes it happens that we forget to format a file. Because of this the next person working on that specific file might have a larger diff due to the additional formatting. As of right now I infrequently format our entire code base and create a PR for that - that’s annoying.
Ideas:

• Use this pre-commit hook so that the dev can’t create a commit if a file is not formatted

• Use this GitHub Action to automatically format our code base on a regular basis

@Mustapha Zorgati

Since Github Action would create a new commit, we decided to use the pre-commit hook.

Code Style: Writing REST Tests

We write our REST tests a little differently. Currently we have two distinctions:
Regarding the HttpEntity:
A)

B)

Regarding the query parameters:

A)

B)

@Mustapha Zorgati

We agreed to extract the URL and the query parameters (suggestion A). The two variables should be named url and auth (authorization).

Polly for the daily

In our retro, there was criticism about participation in our daily. We could introduce Polly which asks every week (e.g. Monday 8 am) about the participation in the following week. What do you think?

@Sofie Hofmann (Unlicensed)

We decided to introduce a polly every sunday.

Workplace Task-Filter

I like to show you different approaches to convert our task-filter to Angular Material since I ran into a few problems with the button layout.

@Sofie Hofmann (Unlicensed)

We divide the workplace into two scenarios: The tasks ordered by workbaskets and the task search over all tasks. This will be done by removing the button to switch between workbaskets/type&value search and replace it by tabs.

Task filter wildcard search

As discussed in the last COP, the filter in our task search should only contain one input field with wildcard search. However, the WildcardSearchField only contains name, description and custom1 - 16. Therefore, we will loose functionality (type and value search). Is this what we want? Should we expand WildcardSearchField?

@Sofie Hofmann (Unlicensed)

We decided that the input field will search for the value and for the wildcard search fields.

New Backlog Date

As already mentioned I would like to move the backlog to another time, because I will be occupied soon on Mondays from 14:00 to 18:00 with personal studying. I would be glad if we could find another possible time slot.

What about Monday 13:00-14:00? Or Tuesday before or after our Coffee/CoP, as it is already our TASKANA day?

@Tim Gerversmann

Our backlog grooming will be every second Tuesday at 4pm.

New release notification

Currently we send out a mail to all stakeholder (personally) to inform about the new release. This is not a scaling approach once we get more stakeholder. I would prefer some kind of opt-in push notification. But I’m not sure, what the best way is.

@Holger Hagen (Unlicensed)

We like to use the Github release notifications: Everyone who wants to know about new releases has to register in Github and choose the Taskana release notifications.

Suggestion for JavaDoc

I would like to discuss with you some of my suggestions for rules in our JavaDoc.

Open

@Tim Gerversmann

We discussed Tim’s suggestions.

Removal of Eclipse instructions in our Docu

In the last COP, the question whether there is still need for Eclipse in our docu (Setup Development Environment) arose. Since nobody of our team currently uses Eclipse, it is hard to write docu for it. On the other hand, there might be other contributors who are not part of our team and want to use Eclipse.

@Sofie Hofmann (Unlicensed)

We decided to remove the docu for Eclipse since its usage has significantly decreased.
https://taskana.atlassian.net/browse/TSK-1632

Pages in Setup Development Environment

While restructuring Setup Development Environment, we were not sure whether to keep those pages or not:

1. Additional settings for Eclipse | Method arrangementarchived

2. Using multiple databases for taskana-core unit tests

3. Testing of the JBoss/Wildfly deployment

@Sebastian Roseneck (Unlicensed)

@Sofie Hofmann (Unlicensed)

1. We remove the Eclipse documentation.

2. We decided to delete its child page and to move the page to Contributor Guide > Testing.

3. This page is also moved to Contributor Guide > Testing.

Replace our Logger

I just stumbled over this article: https://www.yegor256.com/2014/05/23/avoid-java-static-logger.html
What do you think of that? Is it worth it?

@Mustapha Zorgati

We want to implement the logger on our own so we don’t need the dependency.

Curly braces for ThrowingCallable

Currently we always use curly braces for the ThrowingCallable lambda expression. That was necessary because of formatting issues. Those issues don’t (barely) exist anymore. Can we remove those?

@Tim Gerversmann

We decided to remove the curly braces (in case there is only one line code).

Sample data

We have some questions regarding the extension of the test data. We need your opinion on how we should proceed.

@Mustapha Zorgati @Sofie Hofmann (Unlicensed)

We’ll create a new domain for the new samples.

Copyright

We have some questions regarding the introduction of the copyright header. We need your opinion on how we should proceed.

@Mustapha Zorgati @Sebastian Roseneck (Unlicensed)

Mustapha and Sebastian will ask other people from Novatec and will inform the taskana team about the results.

Single Task history Event: Set details for transfer event

Currently the details attribute for a transfer event is always null and instead the “oldValue“ and “newValue“ attributes are used. For the update and create event we only set details. Can we also adapt that for the transfer event to keep it consistent?

@Franziska Bock (Unlicensed)

The details will also contain the transfer event. There will be a patch release for that.

Error Handling

We like to present a suggestion for our error handling.

@Sofie Hofmann (Unlicensed)

We will use error codes which correspond to an error message specified in a json file. In addition, we will display the error messages with https://ngneat.github.io/hot-toast/#examples.

Easier / quicker reviews

Github has a suggestion feature for pull requests. This allows the reviewer to write a code suggestion and allows the submitter to apply those changes in 1 click.

This can safe us a lot of time during our review process. - Let me show you how this works.

@Mustapha Zorgati

Enforce Signed Commits

Do we want to enforce signed commits? What do you think?

https://docs.github.com/en/github/authenticating-to-github/managing-commit-signature-verification

@Mustapha Zorgati

Since there are already many things to do when someone is new to the project, we decided to make signed commits optional.

Invite NT colleagues?

Recently we’ve created an overview of all our NT-internal CoP’s. Should we add this meeting to that list?

@Mustapha Zorgati

Close old PRs?

Do we want to close our old PRs (#1285 & #1193) ? They haven’t been touched by over half a year

@Mustapha Zorgati

We closed both PRs.

Error Handling in Frontend

I want to talk about a few things regarding our new error handling:

1. should we have multiple json files for error/success codes? (keep in mind dialog, warning, info)

2. key or name in messages? (currently, there is a mix)

3. multi-lines or as wide as possible?

@Sofie Hofmann (Unlicensed)

1. We have one file containing
   const messageCodes = { error: {}, success: {}, …}

2. key

3. multi-line

Discuss the Security Workshop

Some of us had a workshop on Security for Beginners. Would you like to discuss some of the points we learned and how we can apply them in TASKANA?

@Sebastian Roseneck (Unlicensed)

We discussed several aspects and made a list of those aspects we'd like to take a closer look at.

Extend TaskQuery with completed and created time frames

Currently we have time frame queries for planned and due: planned-from, planned-until, due-from, due-until. Customers have requested to be able to request Tasks using parameters for completed and created. As TaskQuery.createdWithin() and TaskQuery.completedWithin() already existed, we need the parameters as well available for the Endpoint /tasks

@Chi Nguyen (Unlicensed)

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

How to name Map-Type variables

Currently we have a different approach on naming our Map-Type variables.

There are several discussions like these: https://stackoverflow.com/questions/2253453/how-should-i-name-a-java-util-map
Examples:

• uidToPerson

• valueByKey

@Tim Gerversmann @Mustapha Zorgati

We recommend to use the ‘valueByKey’ pattern in both backend and frontend.

Migrate to Novatec GitHub Organisation?

We have a Novatec GitHub Organization: https://github.com/NovatecConsulting

Should we migrate our repositories to that organization?

@Mustapha Zorgati

We don’t want to migrate to that organization.

Build maven archetypes for taskana adapter example projects

Currently in the TaskanaAdapter Repository there are three example submodules for the reason of documentation. For a potential new customer it’s probably a good idea to give the new customer the chance to generate one of such adapter with the taskana specific files with an taskana maven archetype. One of the examples (taskana-adapter-camunda-listener-example) is already generated with a camunda maven archetype and then afterwards enriched with taskana specific files.

@Sascha Frevel

We discussed about building archetypes for our rest examples and for creating a maven module as alternative for How to properly create a maven module. We’ll create a ticket as a reminder for this topic.

New name for InternalTaskanaEngine#openAndReturnConnection

I am not happy with the name openAndReturnConnection.
My suggestion: executeInDatabaseConnection. Anyone got a better idea?

@Holger Hagen (Unlicensed)

@Mustapha Zorgati renamed this in a current PR.

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

history-SPI: Mapping of task attributes to history event

Currently the mapping of the tasks attributes to attributes of the corresponding history event is handled in the constructor of the history event. An alternative would be to give the task to the history providers which then handle the mapping as it fits

@joerg.heffner

JavaDoc linking field

Currently we have agreed to link attributes of API classes by its getter-method in the following way:

{@linkplain Classification#getId()}

Would it be better to use this:

{@linkplain Classification#getId() id}?

That way it will read for example “The id of the Classification…” instead of “The Classification#getId() of the Classification…” while still linking the wanted getter-method.

@Tim Gerversmann

We prefer {@linkplain Classification#getId() id}.

Escaping variable in format string

I have noticed that we don’t escape parameters in our String.format and logging messages.

Should we do that?

Example:

@Mustapha Zorgati

We prefer the second option and want to change this incrementally.

Defining assert statements for list comparisons

Currently we use different approaches of testing collection-type entities. Sometimes we assert that the list has a specific size and then extract the element(s) we want to test. Sometimes we just extract the element(s) without the assert and sometimes we use the extracting method AssertJ provides. (see example below)
I wish to discuss this and agree on one way to define the assert statements

@Mustapha Zorgati

We want to use the third option (extracting).

Invalid DMN Error handling of the DMN-Taskrouter module

Currently the DMN-router performs a validation of the provided workbasket key/domain rooting output. If this validation fails, the application won’t boot. Should this really be the standard behaviour? I want to discuss some problems that come with this and also problems that might occure when changing this

@joerg.heffner

Was discussed in a separate meeting.

Handling of GitHub notifications

Currently we deal with GitHub notifications differently. Some receive everything, some don’t receive any notification. The issue: We take longer to realize that some PR has been updated and needs a second review.
I have two ideas on how to improve this process. Let’s talk and discuss about them. Maybe be can increase our productivity

@Mustapha Zorgati

We decided that “Participating and @mentions” might be the best setting. This option can be selected in GitHub when clicking “Watch” in the top right corner. We try this out and talk again in case it is not appropriate.

ScheduledJob Type Enum

Currently our ScheduledJob class has an enum called “Type” which contains the different Jobs. I’d like to discuss this enum and possible alternatives.

@joerg.heffner

Mustapha suggests to use a SPI. Details will be discussed in a separate meeting.

Backend Testing Guidelines

The page “Backend Coding Guidelines” contains lots of testing rules. However, there is a child page “Good Testing”. Should we move the testing rules to the child page?

@Sofie Hofmann (Unlicensed)

Additionally, the child page will be divided into unit tests/integration test/REST test/acceptance test/general.

H2 Case Insensitivity

Currently our H2 example database is always run with IGNORECASE=TRUE. Do we want to keep this?

@Tristan Eisermann

We make H2 case insensitive.

assertThat and getters

In our backend tests we sometimes extract the exact object we want to test before the assertThat and sometimes inside it. I would prefer a consistent approach.
Examples:

@Tristan Eisermann

Discussion resulted in no guidelines since readability depends on the use case.
For example, we recommend to use only one get in the parameter of assertThat, so use a variable instead of assertThat(respondeUpdate.getBody().getReceived).
Avoid too many operations after assertThat and keep in mind the syntax assertThat(<testObject>).

Disable LoggingAspect for our releases

Our LoggingAspect has caused some performance issues for our users. Should we disable it and only use it for local development?

@Mustapha Zorgati

We don’t want to disable because a customer wants the LoggingAspect.

Allow Access to SPI Managers without internal Taskana Engine

The PriorityUpdateJob only gets the TaskanaEngine as a parameter, but also has to fall back on a PriorityServiceManager that is only available in the internal engine. We also have an architecture rule that says that ...

 'classes that have name not matching '.*ScheduledJob.Type' and reside in a package '..api..' should only depend on classes that reside outside of package '..pro.taskana..internal..' or assignable to pro.taskana.common.internal.logging.LoggingAspect or assignable to pro.taskana.common.internal.util.MapCreator'

How should the job then get to the corresponding SPI implementation? The customer will certainly not be able to pass an internal Taskana engine as a parameter for scheduling.

@Benjamin Eckstein (Unlicensed)

resetDb for Tests as Annoatation

We have a simple way of resetting the DB before each test to minimize side effects from other tests.

Problem: Performance will suffer if we reset the database for each test even if tests do not modify the database at all.

Current Solution: There is a long waiting todo to split tests into reading & modifying tests in order to enhance performance.

To be discussed: There is another way to solve this problem by using a technique from spring to mark test classes or single test methods as “dirty” with an annotation. We don’t need spring, but could introduce a new annotation “resetContext” and our JaasExtension would handle the rest.
This would remove a lot of code repetition and increase performance.

Now:

Open

Possible solution:

@Benjamin Eckstein (Unlicensed)

In the meeting today at 4pm, we want to discuss how writing tests should look like and whether our new Test API meets this expectations. Based on this discussion, we decide whether we need the annotation suggested here.

Helper Classes

Big classes can be refactored to small classes by extracting logic into smaller more specialized helper classes. This makes understanding and testing easier.

My question is where to put those classes. Do we start with a simple subpackage “helper” in each package if needed? If we will start to re-use those helper classes, we can move them to a more precise and correct package.

@Benjamin Eckstein (Unlicensed)

For now, the classes are in a package called ‘helper’. We postpone the decision on how we can improve this and added the topic to the list of Taskana 5.0 in the channel.

SqlProvider rework

Currently we only provide one way in the SqlProviderUtil to generate SQL-Strings, i would like to present an alternative and get a short vote, if this is a wanted extension.

Example:

SQLProviderUtil Extension:

@Tristan Eisermann

We decided that we want to use the suggested alternative.

applyToQuery() method in QueryPagingParameter

This method doesn’t just apply parameters to a query (which is implied by the name) but also calls the list() methods and therefore executes the query. Should we rename / restructure this method?

@joerg.heffner

We decided to add Paging as Query Parameter and added this topic to the list of the TASKANA 5.0 release

MyBatis SqlProvider

What is the common opinion about the MyBatis SqlProviders? Should we refactor all mappers to use them or not?

@joerg.heffner

SqlProvider make working easier but we don't necessarily need them everywhere.

Testnaming pattern for simple tests

Keep tests name short for simple tests

@Benjamin Eckstein (Unlicensed)

In case it is not possible to name the test with “when” or “for”, it is okay to only write should_ExpectedBehaviour. The most important thing is that the name is as understandable as possible. The result is also written down here Testing Guidelines.

LOMBOK

It’s time to introduce lombok.

Please visit https://projectlombok.org/ and look the 4 Minute Video.

• No more manual Getter, Setter, Equals, Hashcode order toString methods!

• Get Builder classes automatically

• no more manual Logger fields in the class

• no more manual writing of default / standard constructors

• +!! flexibility to ignore lombok and doing it manually for special cases.

@Benjamin Eckstein (Unlicensed)

We want to introduce LOMBOK incrementally. We want to start with getters/setters/hash/equals/toString.

TaskanaConfiguration by annotation

Let us make the configuration great again. There is too much noise and ulgyness … if you want to add a configuration you have to add a lot of code linking the name with a value and a parser together!!! We can archive all of that with an annotation. We could also provide an default value if wanted.

The implementation is already done and available as a PR.

https://github.com/Taskana/taskana/pull/1707

Feel free to have a look make suggestions or enhance the implementation.

Btw. implementing behaviour with annotations is easy, fast and reliable. We should use them more often!

Benjamin Eckstein

PR wird integriert. Weitere Refactorings passieren ohne Ticket in der Zukunft