| Note |
|---|
Do not forget to write down our conclusions in our Guidelines . |
Please be aware:
If a technical lack has been detected, feel free to create a technical debt ticket immediately! It needs not to be discussed before in a CoP by compulsory.
Before a sprint finish, the Android round table should come together to discuss which technical debts are the most important to tackle in the upcoming sprint!
To present
...
Topic
...
Description
...
Presenter
...
Votes
...
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.
...
...
Mustapha Zorgati Comment: ← this has potential and can change how we handle our example projects 
...
Efficiently jumping to tickets
...
I have configured custom search engines (for Google Chrome) in order to jump really quick to some tickets. I can show how I use the custom search engines to access tickets really fast and share my configuration.
...
...
...
Easier / quicker reviews
...
Github has a suggestion feature for pull requests. This allows the reveiwer 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.
...
To discuss
...
Topic
...
Description
...
Creator
...
Votes
...
Selecting CSS classes
...
Currently we use BEM as CSS naming methodology, E.G:div.toolbar.toolbar__buttons.toolbar__buttons-blue
But in CSS we don’t utilize the most out of SCSS capability for BEM. Instead of writing class names in full length, we can use CSS selectors like &. Our example would be toolbar { … &__buttons { … &-blue { … } }}
https://sass-lang.com/documentation/style-rules/parent-selector
...
...
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?
...
...
There is a way to get 1000 minutes travis time for free. If this time is exceeded, there will be costs. Mustapha will collect more information and decide with Holger.
-> After considering the alternatives and costs we decided to have a look if travis can get their issue under control. We will have a look at https://www.traviscistatus.com/#month and talk about how we feel on the 17.11.2020 about an action.
-> 24.11.: Mustapha contacted Travis to collect more information but hasn’t received an answer yet.
...
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:
WITH capital starting letter: Classification
2. WITHOUT capital starting letter: classification.
Both cases can even occur in the same doc or senctence, which reads a little weird.
Should we agree on a convention concerning our entities?
...
...
...
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.
...
...
...
REST Service - Sometimes not consistent
...
I just observed, that we handle update/create of some entities differently.
Request contains an id path variable and request body contains an id aswell.
Sometimes we just override the body variable with the request path variable, sometimes we throw an InvalidArgumentException.
Which way should we do?
Examples: TaskCommentController#createTaskComment and ClassificationController#updateClassification
...
...
...
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 Mustapha Zorgati
...
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?
...
...
...
our group ids
I just discovered that we have a different group id for our history.
...
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
...
...
Presentation Done
...
Topic
...
Description
...
Presenter
...
Outcome
...
Method references vs lambdas
What is easier to read: Method references or lambdas?
Background:
Do we, as a team, want to write method references as often as possible or agree on lambdas?
example:
| Code Block | ||
|---|---|---|
| ||
assertThat(results)
.hasSizeGreaterThan(2)
.extracting(TaskSummary::getWorkbasketSummary)
.extracting(WorkbasketSummary::getId)
.isSortedAccordingTo(CASE_INSENSITIVE_ORDER.reversed()); |
or
| Code Block | ||
|---|---|---|
| ||
assertThat(results)
.hasSizeGreaterThan(2)
.extracting(e -> e.getWorkbasketSummary().getId())
.isSortedAccordingTo(CASE_INSENSITIVE_ORDER.reversed()); |
...
...
Arguments:
First option:
| Status | ||||
|---|---|---|---|---|
|
Second option:
| Status | ||||
|---|---|---|---|---|
|
| Status | ||||
|---|---|---|---|---|
|
Voting result:
First option: Jörg, Bernd, Benjamin, Mustapha
Second option: Holger, Nikita, Sofie
=> We agreed on the first option
...
Walkthrough Buildpipelines
...
Video Session about the building process on Github / Travis and executed scripts
...
...
Recording available
...
Mutation Tests
...
What is it and how can we use it
...
Benjamin Eckstein (Unlicensed)
...
Good solution. we should implement this as soon as possible and learn where we can focus on new tests. We need to fix acceptance tests first. A random subset of acceptance tests will result in failing tests (need DB reset between tests)
...
Architecture Tests
...
What is it and how can we use it see https://www.archunit.org/
Discussion: What architect principles would we like to cover with those tests?
A lot of good examples:
https://github.com/TNG/ArchUnit-Examples/tree/master/example-junit5/src/test/java/com/tngtech/archunit/exampletest/junit5
...
Benjamin Eckstein (Unlicensed)
...
Cool! We need that and it would add great value. Especially before refactoring the core. Implement all the tests we want, if they are not working yet: create a technical debt ticket and disable the test with a reference to this ticket.
...
Good Unittests
...
What unit tests add value and how should we use mocks? Present some info and discuss it. As an example. Look at the unit test testGetTotalNumbersOfCatgoryReport
...
Benjamin Eckstein (Unlicensed)
...
Email address usage in commit messages
...
In the contribution guide is described how to set and use your Email with github. The email you set with git config user.email “some@email.com” will appear in the commit message if you type in git log. So if you don’t want anybody which is cloning you repo can get your email - and is doing some spamming with it - you should use the noreply email which is provided automatically with your github account. You can configure this like in this example
...
if you then use git config user.email “your_account@users.noreply.github.com“ your private or nvatec email will not show up in the git log output or in any other log.
We should also describe this in the contribution guide, because everyone with a github account aloso have this noreply email from github.
All githubb notifications will be send to you as before to the “normal” emails you have entered.
git log example
| Code Block | ||
|---|---|---|
| ||
commit 5ffa41f7930c177cd3c1321b5eba45b078c9f0cb (HEAD -> master, upstream/master, origin/master, origin/HEAD)
Author: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Date: Mon Mar 9 07:41:56 2020 +0000
TSK-1158: disabled forking for Spring Boot process in pom.xml.
commit 470b53f29f0d1278d876ddf56399d93c0479a56d
Author: Jörg Heffner <private email>
Date: Thu Mar 12 14:04:37 2020 +0100
TSK-1150 Comments from Bernd Breier
commit 9a230a3269fd444298e94df54dc9dc4da185b9cb
Author: Jörg Heffner <private email>
Date: Tue Mar 3 11:42:18 2020 +0100
TSK-1150 Java-API for the administration of comments for tasks
commit 0659bb13b02377380213ade04c72a5d70efa5a14
Author: Sascha Frevel <3075075+sfrevel@users.noreply.github.com>
Date: Mon Mar 16 11:41:08 2020 +0100 |
Problem with existing commit messages. The emails there could be changes but this leads in an history rewrite which could be problematic. see https://help.github.com/en/github/using-git/changing-author-info
...
...
Everybody will change his e-mail address and we evaluate the costs of rewriting our history. https://taskana.atlassian.net/browse/TSK-1181
...
Structure of taskana-rest-spring
...
Currently we are restructuring taskana-rest-spring. Before we start our work we want to share our ideas and thoughts so that we have a team agreement for the restructuring.
We want to talk about class-naming, some individual class names and package structure.
...
Christopher Heiting (Unlicensed)
...
https://github.com/Taskana/taskana/pull/1045
...
Running test multiple times with different access Ids
...
Currently we have a lot of tests which are duplicated and only differ in the access Id.
Let me show you how this can be improved
...
...
The class JaasExtension can now be used in different ways.
Multiple @WithAccessId annotations can be used with the @TestTemplate or the @TestFactory annotations.
Note to Testfactory: The first accessId is used to run within the testfactory method.
...
Increase HTML visibility with comments
...
HTML Code in Taskana is often hard to skim through without a deep understanding. To improve the visibility, each big section in HTML can be divided with a comment.
EG:
<!-- HEADER -->
<div> … </div>
<!-- TASK LIST -->
<div> <ul> … </div>
Comment: Benjamin Eckstein (Unlicensed)
Always try to explain yourself in code. Don’t add superficial HTML comments. Use the ID attribute or class names to make <div id=tasklist> or <div class=list tasks”> divs more understandable. You can also always use <sections> with a role attribute. Or be cool & modern and use custom HTML tags (see: https://www.html5rocks.com/en/tutorials/webcomponents/customelements/ )
Comment: Chi Nguyen (Unlicensed) If everything is done correctly after the clean code principle then this suggestion wouldn’t be needed
...
...
We will structure our HTML files by using this notation.
There will be a dedicated ticket for the administration component: https://taskana.atlassian.net/browse/TSK-1222
The other components will be changed on the fly.
...
RepresentationModel getter/setter vs public fields
...
During the refactoring of the assemblers within taskana-rest-spring we found multiple solutions for the goal implementation.
Let us show you what we have for ideas and find a solution which fits everyone.
...
Mustapha Zorgati / Christopher Heiting (Unlicensed)
...
Outcome of discussion about private/public fields in the RepresentationModel classes: The fields are not changed to public, instead they remain private with getter/setter methods.
...
Auto-comment bot with checklist & current release notes
...
We introduced an auto-comment bot which shows a checklist of our team agreements when creating a pull request,. This checklist contains both todos for the submitter and todos for the reviewer. Furthermore, there are two new pages with our current release notes for taskana and the adapter. After every PR, this page should be updated with a description of the changes. We want to talk about the categories we’ve introduced where the changes can be filled in.
...
Sofie Hofmann (Unlicensed), Mustapha Zorgati
...
We decided that we should not update the current release notes after every PR. Instead, only the important changes should be written down there.
...
Research: Refactor MyBatis mapper classes
...
Currently our MyBatis mapper classes have huge sql scripts (100+ loc). This, and the lack of syntax highlighting is really annoying and decreases productivity.
In order to increase maintainability we want to find out a way to generate those sql scripts in a more elegant way.
There is currently a PoC here: https://github.com/nkolytschew/taskana/tree/nko-1290_mybatis-poc
Please check SQL and String Script generation for further discussion.
...
...
We decided for the string builder. This makes the code reusable and testable. In addition, the string builder is easier to understand for people with less experience than the SQL builder is. However, splitting up the code in too many pieces can make the code unreadable again. That’s why we have to learn a tradeoff between avoiding duplicate code by writing methods and keeping readability by not splitting the code in too many fragments.
Nikita will start with the ClassificationQueryMapper and find out an appropriate level of granularity together with the team. He will also come up with suggestions about testing. The solutions will be transferred incrementally to the other mapper classes.
Discussion Done
...
Topic
...
Description
...
Creator
...
Actions
...
DBWriter in history module
...
The DBWriter in the taskana-simplehistory-provider module currently sets up test data for the HISTORY_EVENTS table (and ONLY for this table). There is a lot of duplicated code from SampleDataGenerator of the taskana-data module.
Also the DBWriter seems faulty. Should we add a dependency to taskana-data in test scope and use the SampleDataGenerator instead?
...
...
JaasExtension and WithAccessId/s
...
The JaasExtension class and the WithAccessId/s interfaces are located in the test package of taskana-core. Therefore they can’t be used in other modules. (Even if you have a dependency to core)
This makes it hard to test things e.g. in the history module. Currently you have to test without security.
Should we keep testing with an unsecured engine in these cases? This might not always be possible.
Should we copy/paste JaasExtension class and the interfaces?
Should we create a separate module which will contain them which then can be added as a dependency in test scope where it’s needed?
...
...
https://taskana.atlassian.net/browse/TSK-1277
...
DB Versions
...
Should we upgrade our database versions? Postgress, DB2, H2?
...
...
Postgress: 10.X => TSK-1197
DB2: 11.1.4.4
...
HTTP Status Code return type
...
All our *NotFoundExceptions return a status code 404 (NOT FOUND). The DomainNotFoundException returns a status code 400 (BAD REQUEST).
Why do we have this special behaviour for the Domain?
Comment from Holger Hagen (Unlicensed) -
my understanding is, that 404 only applies to the resource identified by the URI. If we post a new resource which depends on something which does not exist, a 400 return code is fine.
See https://restfulapi.net/http-status-codes/ -
“400 […] Errors can be like malformed request syntax, invalid request message parameters, or deceptive request routing etc.”
“The 404 error status code indicates that the REST API can’t map the client’s URI to a resource but may be available in the future. “
...
...
See Guidelines → Backend → Http status code return type
...
Creation of “How to develop”
...
We have a lot of findings from this meetings. Should we start document our decisions somewhere and add it with our development rules?
...
...
=> Guidelines
...
Proper usage of assertj
...
Currently we have a lot of tests which are using asserts in a very complicated way and thus result in harder readable tests.
Let me show you some examples and their improvements.
...
...
We always use assertThat(<test object>) only. Never have a function call within the parenthesis.
See Guidelines
...
assertJ Migration
...
I just found this script: https://github.com/joel-costigliola/assertj-core/blob/master/src/main/scripts/convert-junit5-assertions-to-assertj.sh
This will allow us to automatically migrate from Junit5 → assertJ without little effort.
Thoughts?
Precondition: we have to eliminate hamcrest first.
...
...
https://taskana.atlassian.net/browse/TSK-1180
...
Disabled Tests
...
Currently we have 10 disabled Tests. How should we deal with them? Remove or invesitgate?
Update: Not 10 anymore …
...
...
https://taskana.atlassian.net/browse/TSK-1182
...
Naming convention for tests
...
Currently different developers used different naming conventions for tests.
For example the Should_ExpectedBehavior_When_StateUnderTest convention (see TaskQueryTestImpl) or the test[Feature being tested] convention (see TaskAttachmentTest) or in the adapter with a lot of _ between words
Should we define a standard convention for everyone to stick to? If yes, does this apply only for new tests or should old test be "migrated"?
...
...
Test pattern for new tests:
should_ExpectedBehavior_When_StateUnderTest
...
Dedicated taskana travis github user
...
Currently we have a dedicated taskana github travis user (https://github.com/taskanaTravis ) which is supposedly used to write back the poms with the new versions to the taskana main repository after a relese. Unfortunately this has never really worked, because of reasons and nobody really cared. (see https://github.com/Taskana/taskana/commit/c90d1f903bc50776be7f4210fa676a46d31227fc for example)
Can we just delete that account and just use a github access token from Holger Hagen (Unlicensed) to allow travis to push commits in Holgers name.
This is done in about 5 Minutes (literally changing a travis environment variable..)
...
...
We deleted the dedicated github user and used Mustapha Zorgati github credentials for travis
...
SonarQube
...
We finally got SonarQube set-up. Unfortunately currently it cannot run for PRs.
Lets discuss how we want to integrate SonarQube in our current workflow
...
...
Everyone has to run his own sonarcloud instance and put a branch analysis link into their PR. See SonarCloud Integration
...
Definition of Reviews
...
Discussion of Review Process:
What & What not to review.
What is the goal? Knowledge Sharing, Bug Prevention? Catch unapproved changes? Solving possible Problems before merging?
Who reviews? One or many or everyone? Only People working on similar tasks?
How to comment on PR?
When to approve a review? Fulfilling a checklist? Solving all comments?
Is a Review needed if the PR was done by pair programming?
How can we minimize the waiting time for PR? Idea: Do PR always first thing of the day, then continue working on tasks.
If you want a deep meaningful discussion please view this awsome talk first:
https://www.youtube.com/watch?v=jXi8h44cbQA
...
Benjamin Eckstein (Unlicensed)
...
Dedicated meeting for this on 24.01.2020.
The results are in our guidelines.
...
Semantic versioning in web frontend
Currently we use semantic versioning in the web frontend (^ and ~ in front of version). Because of this and the package-lock.json file sometimes the build breakes due to version mismatch with build and the package-lock file.
How should we deal with this?
...
...
Remove semantic versioning and stick with static versions for now (until our test coverage in the frontend is good enough and we’re confident that it’ll catch errors from version updates)
https://taskana.atlassian.net/browse/TSK-1036
...
Using multiple Assertions framework
...
Currently we are using both Junit5 Assertions and hemcrest assertions.
Personally I think that using multiple framework for assertions in tests is not “clean”. We should discuss which one we should use in the future. Also Consider AssertJ
https://www.blazemeter.com/blog/hamcrest-vs-assertj-assertion-frameworks-which-one-should-you-choose/
...
...
https://taskana.atlassian.net/browse/TSK-1025
...
Modified timestamp
Currently we are using the modified timestamp in our entities to determine whether an entity has been modified by a different user. If the requests are processed in the same millisecond this method will not catch anymore. Thus we override the previous changed entity.
How serious is this? Is it worth checking out Etags?
...
...
Spike it. How much effort? Is it worth it?
https://taskana.atlassian.net/browse/TSK-1026
...
Testing session before releases for the UI
...
Should the whole team take some time to test the UI and all functionality before releases?
...
...
Outcome: explorative testing appointment within each release. but don’t forget to test frontend after merging.
...
Fix race conditions in tests
...
How should we deal with race conditions in integration tests?
Example: We have a test class that is deleting objects in the DB on some methods and counting a result in another method. The result of the list count depends on the execution order.
Idea: Split reading and modifying DB Tests. Reset DB @beforeeach for modifying test classes and @beforeall on reading.
...
...
Create a ticket and implement the test splitting of reading and modifying test cases.
https://taskana.atlassian.net/browse/TSK-1027
...
Rename Module spring-base
...
The Module’s purpose is to serve as a login (stub) service. Naming it “base” is confusing
...
Benjamin Eckstein (Unlicensed)
...
https://taskana.atlassian.net/browse/TSK-949
Modules will be renamed.
...
Dependencies
We should find a way to update our dependencies. How should we tackle this issue?
...
...
Outcome: Test for known security vulnerabilities and let the build fail if found.
Take care of incompatibilities with the customer. What about different spring boot versions?
...
dedicated test modules.
...
Currently, our rest and history applications are split very “uncommonly”. Why do we have this split and is it useful?
...
...
Outcome: Ask Holger for the Reason. Maybe we can merge rest-spring-test with rest-spring.
...
Problematic dependency between TaskanaAdapter and TASKANA
Changes in TASKANA can easily break the adapter. This recently became especially obvious while refactoring TASKANA. How do we want to handle this in the future?
...
O1: The snapshot version of the adapter could use a fixed version of taskana
O2: Combine Adapter and Taskana into 1 Project, run maven / Travis to test both
O3: Do nothing, refactoring or breaking changes do not happen that often. Manual fixing if we notice a broken build.
...
Mustapha Zorgati & joerg.heffner
...
Voting Results:
O1: Benjamin Eckstein (Unlicensed) Holger Hagen (Unlicensed)
O2:
O3: Bernd Breier (Unlicensed) Holger Hagen (Unlicensed) joerg.heffner
The result is Do Nothing.
...
Length of Data Types
...
With the adaptation of the new HATE terminology the length of our data types has exceeded our maximum line length.
...
This currently forces us to ignore the checkstyle warnings in these cases and needs a better solution.
...
Christopher Heiting (Unlicensed)
...
No action required since the formatter solves the problem automatically.
...
Commit Messages
...
Sometimes I find some commit messages with this format “Comments from X”.
These do not match our current standard for commit messages.
...
...
See Guidelines. The reviewer of the PR has to check the commit messages.
...
Use region to logically group and fold code
...
Currently we have a lot of huge classes which contain many methods.
In order to achieve more structure and readability we can use code folding. What do you guys think about that? Look at JaasExtension for example.
Question: what about or Eclipse colleagues?
...
...
We will use code folding in case it seems to be appropriate.
| Status | ||||
|---|---|---|---|---|
|
SF: have a look on this page Setup Developer Environment for the plugin description.
...
Removal of pro.taskana.common.api.LoggerUtils
...
What’s the reason for this class? All methods within that class can be replaced with the standard toString implementation for that entity. See the list below for details
listToString + setToString => https://docs.oracle.com/javase/8/docs/api/java/util/AbstractCollection.html#toString--
mapToString => https://docs.oracle.com/javase/8/docs/api/java/util/AbstractMap.html#toString--
...
...
The class LoggerUtils will be removed.
Sascha Frevel will create a ticket.
https://taskana.atlassian.net/browse/TSK-1247
...
Version updates of dependencies also included in Spring Boot
...
Is it a good idea to update every dependency (like Spring framework) as soon as it gets available, or would it be better to update all Spring dependencies at once as soon as a new Spring Boot version is available?
...
...
Sascha Frevel will create a ticket.
https://taskana.atlassian.net/browse/TSK-1258
...
Empty AttachmentController
...
The class AttachmentController is currently empty and has a single usage.
Can we and should we remove it?
...
Christopher Heiting (Unlicensed)
...
Delete the class.
...
Maven Profiles
...
What are the profiles used for? Are they all necessary? Can we name them better?
...
Mustapha Zorgati & Tobias Baden
...
| Code Block |
|---|
Profile Id: coverage (Active: false , Source: pom)
Profile Id: eclipse (Active: false , Source: pom)
Profile Id: history.plugin (Active: false , Source: pom)
Profile Id: postgres (Active: false , Source: pom)
Profile Id: release (Active: false , Source: pom)
Profile Id: snapshot (Active: false , Source: pom) |
Mustapha Zorgati will delete eclipse and postgres is renamed to wildfly-example.postgres
...
Documentation update bot
...
The bot asks, if the documentation has been updated. The agreed on response is a 
, which does not feel like an sufficient answer, if the documentation did not need updating.
Perhaps we should distinguish between “updated” and “not needed“.
...
...
| Status | ||||
|---|---|---|---|---|
|
...
Removal of asSummary()
...
Due to the new structure of the RepresentationModelAssembler classes the asSummary() method is no longer in use nor needed.
Should we delete it or reimplement it in the new structure?
...
Christopher Heiting (Unlicensed)
...
asSummary() can not be deleted since we need this method for the equals check: equals is a symmetrical function and would return false in case there is no asSummary().
...
Working with Forks / SonarQube
...
What is your experience working with a SonarCloud instance for each fork?
Currently I feel that we have a lot of overhead and can’t really trust the quality gate from the fork instance since the settings are not applied to it.
Depending on the result of the previous question:
Should we change our process and work on Taskana/taskana instead of our forks?
How can we improve further regarding code quality?
...
...
We decided against the change of the working process since we prefer working with forks.
As a reviewer, check the link to sonarcloud analysis and possibly do not approve.
In case it is merged, mark the new issue as resolved after merging.
...
New Select And Claim API
...
How would you prefer the new select and claim API to be implemented:
New method on the Taskservice-Interface selectAndClaim with a TaskQuery parameter
Directly inside the TaskQueryImpl single() method as a special case
Advantage case 1: All other write operations that modify a task are offered through the Taskservice. → We stay consistent with the current programming model
Disadvantage case 1:This scenario needs to be performed in two steps: creating a query (and not executing it) and call the selectAndClaim method. Might feel less comfortable as a developer.
Advantage case 2: This scenario can be performed in one single step: Create and execute the TaskQuery as usual.
Disadvantages: The TaskQuery is not meant to perform write operations, as the name already suggests
...
...
After voting, we decided for option 2.
...
Exceptions in JunitTests
...
currently we have two ways of dealing with Exceptions in Unit Tests:
list all Exceptions individually
make the test throw Exception.class
We should unify this and make the result a guideline.
In Order to get an Idea use the following regex Expressions to find examples:
.* throws (\w+Exception,? *)+ \{, file mask: *Test.java.* throws Exception \{, file mask: *Test.java
...
Instead of listing all exceptions individually, we will throw Exception.class.
https://taskana.atlassian.net/browse/TSK-1318
...
Dependency management for often used dependencies
...
Currently we are managing our dependency versions in properties of the root pom. What if we have dependencies used by every single module, like junit: Should we keep them in every module pom, or start creating a pom hierachy to manage those dependencies better.
Is this even something to discuss, or am I overthinking it?
...
...
We won’t change the current dependency management.
...
Autowiring Constructors
...
Should we use the annotation @Autowired for our autowired constructors or not?
...
...
We use @Autowired annotation.
...
String constants for debug messages
...
Sonarcloud reports a duplicate warning if we use the same message in our logger statements.
Do we really want to use constants?
...
...
We won’t use constants in logs.
However, Benjamin Eckstein (Unlicensed) pointed out that this is a hint on code duplication.
...
Personal feelings about code in PRs
...
In our Code Review guideline we said that personal feelings about clean code are not part of the review.
In practice I experienced that the clean code tips I got from the team are very valuable in order to grow.
What do you think? Should we handle this point differently during reviews?
Suggestion: Prefix personal feelings with PERSONAL: and make that change optional, not mandatory?
...
Mustapha Zorgati / joerg.heffner
...
We allow personal feelings about clean code in our reviews whereby we will consider these comments as suggestions. Submitter and reviewer should agree on a solution. If you consider the result valuable, share it with the colleagues in the community of practices. Furthermore, if submitter and reviewer can not agree on a solution, discuss about the issue in the CoP.
...
Types in Lambdas
...
Should we, in order to increase readability, enforce types in our lambda statements?
example Backend:
| Code Block |
|---|
(String sortBy, SortDirection sortDirection) -> {
// bla bla
} |
over
| Code Block |
|---|
(sortBy, sortDirection) -> {
// bla bla
} |
example Frontend:
| Code Block |
|---|
this.categoryService.getClassificationCategoriesByType().pipe(
take(1),
tap(classificationTypes: CategoriesResponse => {
// bla bla
}),
); |
over
| Code Block |
|---|
this.categoryService.getClassificationCategoriesByType().pipe(
take(1),
tap(classificationTypes => {
// bla bla
}),
); |
Note Chi Nguyen (Unlicensed) : with the coming styling PR this wouldn’t be needed since the IDE will do it automatically
...
...
We decided to not enforce types in our lambda statements.
...
Variable naming conventions
...
Do we need naming conventions for all kind of variables? (parameters, instance variables, class variables … )
...
...
The variable names have to be readable and understandable. There are no further restrictions or conventions except that.
...
End-to-End testing
...
Our current tests do not test the endresult our product produces. This leads to interesting problems, as it is possible, that all tests are successful, but the frontend wont show you anything but errors. ( Wrongly named selectors lead to this, as they do not get tested )
In our PRs we do not require manual testing, which could lead to a situation, where our bluemix will be completly unavailable.
I do not suggest we implement thorough e2e testing, but crude and unrefined automated testing could be beneficial in avoiding this.
...
...
We decided for e2e testing. Tristan and Jörg will integrate Tobias' tests after the next release.
...
Preparation for more users
...
Currently we have three major problems while developing:
Structure: TASKANA grew historically. We always added functionality without properly reviewing its structure and reworking on it. This results in classes (TaskServiceImpl I am looking at you) which are way too big and complex
Tests: All our tests are integration tests. This is not what integration tests are used for. We once talked about our test structure. Currently it is extremly annoying to write tests since all our tests are dependent from each other.
Documentation: Currently our documentation is poor (from the user and technical perspective). With increasing demand of TASKANA we should start worrying about our documentation Structure to improve usability and increase productivity. An idea would be arc42.
Is this the right time to take a step back and start preparing for new feature sets and users?
...
...
The documentation has the highest priority. We want to talk about changes of the wiki and javadoc in the next backlog refinement (10.08.).
Concerning the structure, we want to rework the TaskServiceImpl class:
| Status | ||||
|---|---|---|---|---|
|
| Status | ||||
|---|---|---|---|---|
|
The tests have the lowest priority.
| Status | ||||
|---|---|---|---|---|
|
...
String.format() or concatinations?
...
Currently both are used in the backend. Should one be preferred or avoided over the other?
Aspects we may want to consider:
Localisation
Performance
Readability
...
...
Since String.format() makes code more readable but decreases performance, we prefer String.format() if the code is not constantly executed. Additionally, we don’t want to use String.format() in logging statements.
...
Module taskana-simpehistory-rest-spring-example
...
Do we really need this module?
...
Mustapha Zorgati / joerg.heffner
...
| Status | ||||
|---|---|---|---|---|
|
...
Do we want to use the assertJ soft assertion feature?
...
AssertJ offers a way to softly assert everything and get a detailled error message for every error and not fail for every single assertion. https://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html#soft-assertions
I see that this can be useful when working with complex datastructures (like reports).
...
...
When we will write new tests or edit old ones, we want to use the assertJ soft assertion feature.
...
Separate logic from assertions?
...
We have a testcase where I did the following assert:
| Code Block |
|---|
assertThat(taskService.getTask(taskId).getState()).isEqualTo(TaskState.CLAIMED); |
Should we consider always separating the business logic calls, to keep the asserts as small as possible? Does this make it more readable? Example:
| Code Block |
|---|
String eventType = historyService.getHistoryEvent(listEvents.get(0).getId()).getEventType();
assertThat(eventType).isEqualTo("TASK_CLAIMED"); |
Should we make a general decision or should we say e.g. if the assert with bussiness logic fits into 1 LOC it’s fine and readability should be implied?
...
...
We only want to introduce a new variable if the number of operations in an assertion / expectation (backend / frontend) exceeds 2 or 3. This is not a fixed rule yet - we want to evaluate this approach in a few weeks.
Additionally, Mustapha pointed out that assertJ offers a fluent API with the purpose to achieve better readability. Therefore, methods like the extracting-method should be preferred if it’s possible.
...
Upgrade to Java 11
...
While upgrading to Java 11 there were several issues, for example:
Postgres doesn’t support microsecond precision for Instants, while Instant.now() will return microsecond precision from Java 9 and on.
The Group Interface is deprecated
I would like to present some possible solutions and discuss other ideas
...
...
Since Postgres does not support microsecond precision for Instants, we will truncate the Instants in the setters of all entities.
GroupPrincipal will implement Principal instead of the Group Interface which is deprecated from Java 9.
Furthermore, we will use Travis to ensure that both Java 8 and 11 work.
...
Behaviour of the single method in queries
...
joerg.heffner discovered an issue in the query api: https://taskana.atlassian.net/browse/TSK-1355
I would like to discuss this briefly with you to make sure that we all have the same understanding.
...
...
We decided for the most common solution as Holger mentions here. The one who calls the query must be sure that only one object will be returned. Otherwise, an exception will be thrown. We have to make sure that our java doc is correct.
...
Module folder structure
...
Personally I think that the module folder structure is not very good.
Especially regarding expansion in TSK-1277 (taskana-test-common).
Does anyone have a different idea?
...
...
We will add a ‘common’ folder on the lib / rest / history level. This new folder will contain taskana-common-test and taskana-data. Taskana-data will be renamed to taskana-common-data. With these changes we want to improve readability and prevent the growth of the lib folder. We will not move all examples to an ‘examples’ folder since the examples should be in the same folder as the corresponding code.
...
Engage as Maintainers in Hacktoberfest
...
Hacktoberfest is a month long celebration for open source software.
During October developers and companies are asked to contribute to open source projects. When someone reaches 4 PRs in a hacktoberfest project they’ll get a free t-shirt.
Should we engage with this event and open TASKANA for the community in hope that someone will contribute and expand our reach?
...
...
Mustapha Zorgati will do the organization so Taskana can participate. Mustapha, Holger and Jörg will collect simple tickets which are made available to the hacktoberfest project.
...
Possible fix for our auto-comment bot
...
I have a solution for the auto-comment bot. But I want to talk about an alternative to a bot:
pull request templates. With this we might achieve the same thing as the bot, but don’t rely on a third party system anymore.
Further details: https://docs.github.com/en/free-pro-team@latest/github/building-a-strong-community/about-issue-and-pull-request-templates#pull-request-templates
...
...
We decided to use the pull request template. Ticket incoming.
...
Truncating of all Instant types
...
THIS IS CURRENTLY BLOCKING OUR MIGRATION TO JDK11. Let’s please prioritize this
Recently we discussed that we want to truncate all our Instants to microseconds in order to keep database compatiblity. Our first attempt was to modify the behaviour of our Pojo’s. I have an alternative solution. Let us talk about it please 
...
We agreed on keeping the way we discussed earlier and modify the Instant in our POJOs.
...
Constants in our REST-Mappings
In our mapping for the REST endpoints we have several constants that get put together from other constants and additional individual content.
Example:
| Code Block |
|---|
public static final String PRE = "/api/v1/";
public static final String URL_CLASSIFICATIONS = PRE + "classifications";
public static final String URL_CLASSIFICATIONS_ID = URL_CLASSIFICATIONS + "/{classificationId}";
|
Should we keep it like it is or change the constants to not contain other constants in favor of a better readability?
...
...
We agreed on expanding our Strings in order to increase readibility
https://taskana.atlassian.net/browse/TSK-1430
...
Rename DocumentationTests?
...
Currently all our test classes which generate the Documentation are called XXXDocumentation.
Should we rename them to XXXDocumentationTest?
...
...
We will rename these test classes to XXXDocumentationTest.
https://taskana.atlassian.net/browse/TSK-1436
...
Rename master branch
...
There is a move in the internet that the name git branch master is bad (e.g master/slave) and thus should be renaimed. A lot of people startet naming their old master branch main .
I can’t find links which explain this movement a little bit.
Since this is no effort and github already changed the default branch name we can follow aswell. https://github.com/github/renaming
...
...
We will not rename our master branch for now. We will talk about this again if the topic becomes more urgent.
...
Which artifacts to release
...
In the past we released a lot of artifacts by accident. This is probably caused by multiple reasons.. One reason might be where we declare the artifacts which should be released.
I want to show you where you can find the artifacts which are released and the strategy to define them. There are two options for this
declare all artifacts which should be released
declare all artifacts which should NOT be released
Currently we are using the second strategy. Personally I think that was a factor why we released.
...
We will declare all artifacts which should be released.
https://taskana.atlassian.net/browse/TSK-1438
https://taskana.atlassian.net/browse/ADPT-112
...
Checked vs Runtime Exceptions
...
Modern best practices recommend avoiding checked exceptions to have cleaner code. Checked Exceptions are only recommended when recovery is possible. Also instead of dealing with different Exception classes add a type/code property to reduce needed catch blocks. This avoids using an all catcher (like Exception e).
...
Ergänzung - Meinung von Axel:
“ganz klare Tendenz weg von checked exceptions. Neuere Sprachen wie Kotlin implementieren das schon gar nicht mehr (mit guter Begründung: https://kotlinlang.org/docs/reference/exceptions.html#checked-exceptions).
Kurz zusammengefasst ist das Hauptproblem, dass die Verankerung der Exception in der Methodensignatur eine starke Kopplung innerhalb der Aufrufketten verursacht. Das ist besonders bei nicht mehr ganz kleinen Mengen an Code unangenehm und lässt auch gute Teams vor Refactoring zurückschrecken. In Java empfehle ich, eine checked exception sofort als RuntimeException zu wrappen.
Wenn ihr eine entwicklerfreundliche API entwerft, dann dokumentiert die Exceptions gut, ggf. mit minimaler Typhierarchie und einem Basistyp, und macht sie bitte alle unchecked.”
...
Benjamin Eckstein (Unlicensed)
...
We don’t want a general conversion to unchecked exceptions.
The decision between checked and unchecked exception should depend on the use case e.g. the ConcurrencyException and the AttachmentPersistenceException could be converted to unchecked exceptions. Additionally, we could summarize exceptions and add an identifier for the context e.g the NotFoundException could be extended with the field ‘component’ and then the specific expections like the TaskNotFoundException could be dropped.
...
Delete unused method?
...
AttachementHandler#augmentAttachmentsByClassification is not used anymore. Shall we delete it?
Method was introduced by Bernd Breier (Unlicensed) on 09.03.2020 in this commit during TSK-1143
...
...
Mustapha and Jörg will look at this after the COP.
...
JDK11 - what does that mean for us?
...
We finally upgraded to jdk11 with backwards compatibility to jdk8. What does this exaclty mean for us?
...
...
In the ‘getting started’ of our documentation should be a hint that we have to ensure downward compatibility to Java 8. So we have to pay attention that we don’t use functions which are only available in Java 11. https://taskana.atlassian.net/browse/TSK-1455
...
Update node version
...
Currently we are running on node v12. LTS is 14. Can we easily upgrade?
...
...
Chi Nguyen (Unlicensed) will update the node version.
https://taskana.atlassian.net/browse/TSK-1453
...
Maven Wrapper
...
Should we get maven-wrapper for our repositories?
Is there something equivalent for node?
...
...
We decided to introduce the maven-wrapper. This means we won’t have to install Maven on our local systems anymore and everyone uses the same Maven version. We have to document the wrapper in our ‘getting started’ documentation and Tim Gerversmann will link this wherever in the docu Maven is used. https://taskana.atlassian.net/browse/TSK-1456
...
JDK for demo environment
...
Currently our demo environment (cloudfoundry) is using jdk8 to run our application. Should we update this to jdk11 too?
Note: Our build pipeline is only testing wildfly with jdk8.
...
...
Currently, we don’t update to jdk11 since there are no effects.
...
Rename CloudFoundry organization
...
Currently the organization is “NovaTec Consulting GmbH”. According to our company guidelines the t is not capital anymore. Can we easily change that?
...
...
We don’t rename the organization.
...
Report Endpoints
...
The Naming of our reports is inconsistent. Can we tidy this up?
...
...
There are inconsistencies in our URLs, Java entities and rest docu which should be removed.
https://taskana.atlassian.net/browse/TSK-1465
...
JavaDoc Guidelines
...
Everyone has a different style of writing javadoc comments. Should we start getting some guidelintes aswell?
Example for Exceptions: Some people start the sentence with an if , others with a when.
...
...
If we see a language mistake or an inconsistency in the JavaDoc while working on a PR, we can change it in a separate commit. Additionally, we should be aware that JavaDoc is part of a review.
Mustapha collects good practices for JavaDoc and writes them in the guidelines. We also have to collect good practices for the frontend documentation.
Test structure consistency
...
Monolithic tests
...
Our test structure seems to be not quite consistent. For some tests we have a single Tests class for all kinds of use-cases and for others, there are numerous classes for different use-cases.
What do we prefer? Should we agree on a general rule?
One example where this might bloat up our tests is attached below:
...
+
We have some tests, that test a lot of different use cases in 1 single test. E.g. testing different IN or LIKE parameters for querys.
Is that okay for us? Or should we agree on putting each use case into its own test?
...
...
We want to discuss these two topics in a separate meeting (~ 60 min) to collect suggestions and find a solution.
The meeting will be on 26.11. and Jörg will prepare and moderate the meeting.
...
Add jest-when to web app
...
jest-when is a utility test library that helps writing tests for jest easier. It is classed as ADOPT according to the latest thoughtworks techradar. Instead of writing long mock functions to be called in tests, you can directly call
const fn = jest.fn()when(fn).calledWith(1).mockReturnValue('yay!')expect(fn(1)).toEqual('yay!')
when(fn) .calledWith(1).mockReturnValue('yay!') .calledWith(2).mockReturnValue('nay!')expect(fn(1)).toEqual('yay!')expect(fn(2)).toEqual('nay!')
https://www.npmjs.com/package/jest-when
...
...