Do not forget to write down our conclusions in our Backend Coding Guidelines and Frontend Coding Guidelines as well.
Topic | Description | Creator | Actions |
|---|---|---|---|
JCI | Travis is having a lot of performance issues lately. Can we do something about this? | We migrated to github actions. | |
Clean Classification interface | Classification interface has methods which are already extended by ClassificationSummary interface. | ||
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. | 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? | 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 | 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!!!)  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. | 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. | Since large Json strings are not maintainable und readable, we want to use the objectMapper to convert the Java object into the string.
TSK-1537 - Getting issue details... STATUS | |
| I often see our entities like task, workbasket, classification etc written in two different ways wihtin java doc and/or comments in the code:
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? | 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 |
| |
Task search in workplace |
|
| |
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 | 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. | 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. |
| |
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? | fixed: TSK-1571 - Getting issue details... STATUS | |
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. | 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 I see three options:
For idea 1 and 3 we have to take account how that changes our currently existing artifacts in maven central |
| |
| 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.
| 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: HttpEntity<String> request = new HttpEntity<>(restHelper.getHeadersAdmin());
ResponseEntity<ReportRepresentationModel> response =
TEMPLATE.exchange(
restHelper.toUrl(RestEndpoints.URL_MONITOR_TASKS_STATUS_REPORT)
+ "?workbasket-ids=WBI:100000000000000000000000000000000007"
+ "&states=READY&states=CLAIMED",
HttpMethod.GET,
request,
ParameterizedTypeReference.forType(ReportRepresentationModel.class));
B) ResponseEntity<ReportRepresentationModel> response =
TEMPLATE.exchange(
restHelper.toUrl(RestEndpoints.URL_MONITOR_TASKS_STATUS_REPORT)
+ "?workbasket-ids=WBI:100000000000000000000000000000000007"
+ "&states=READY&states=CLAIMED",
HttpMethod.GET,
new HttpEntity<>(restHelper.getHeadersAdmin()),
ParameterizedTypeReference.forType(ReportRepresentationModel.class));
Regarding the query parameters: A) String parameters = "?workbasket-ids=WBI:100000000000000000000000000000000007"
+ "&states=READY&states=CLAIMED";
ResponseEntity<ReportRepresentationModel> response =
TEMPLATE.exchange(
restHelper.toUrl(RestEndpoints.URL_MONITOR_TASKS_STATUS_REPORT)
+ parameters,
HttpMethod.GET,
new HttpEntity<>(restHelper.getHeadersAdmin()),
ParameterizedTypeReference.forType(ReportRepresentationModel.class));
B) ResponseEntity<ReportRepresentationModel> response =
TEMPLATE.exchange(
restHelper.toUrl(RestEndpoints.URL_MONITOR_TASKS_STATUS_REPORT)
+ "?workbasket-ids=WBI:100000000000000000000000000000000007"
+ "&states=READY&states=CLAIMED",
HttpMethod.GET,
new HttpEntity<>(restHelper.getHeadersAdmin()),
ParameterizedTypeReference.forType(ReportRepresentationModel.class));
| We agreed to extract the URL and the query parameters (suggestion A). The two variables should be named
| |
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? | 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. | 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 | 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? | 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. | 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. | 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. | We decided to remove the docu for Eclipse since its usage has significantly decreased. | |
Pages in Setup Development Environment | While restructuring Setup Development Environment, we were not sure whether to keep those pages or not: |
| |
Replace our Logger | I just stumbled over this article: https://www.yegor256.com/2014/05/23/avoid-java-static-logger.html | We want to implement the logger on our own so we don’t need the dependency.
| |
Curly braces for | Currently we always use curly braces for the | 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. | 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 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? | 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. | 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 This can safe us a lot of time during our review process. - Let me show you how this works. | ||
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 | 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? |
| |
Close old PRs? | Do we want to close our old PRs (#1285 & #1193) ? They haven’t been touched by over half a year | We closed both PRs. | |
Error Handling in Frontend | I want to talk about a few things regarding our new error handling:
|
| |
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? | 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 |
TSK-1656 - Getting issue details... STATUS | |
How to name Map-Type variables | Currently we have a different approach on naming our Map-Type variables. Map<String,Parameters> HOW_SHOULD_I_BE_NAMED_? = ...; There are several discussions like these: https://stackoverflow.com/questions/2253453/how-should-i-name-a-java-util-map
| 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? | 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. | 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 | Mustapha Zorgati renamed this in a current PR. TSK-1649 - Getting issue details... STATUS | |
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 |
| |
JavaDoc linking field | Currently we have agreed to link attributes of API classes by its getter-method in the following way:
Would it be better to use this:
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. | We prefer
| |
Escaping variable in format string | I have noticed that we don’t escape parameters in our Should we do that? Example: // current
String.format("ObjectReference %s of %s must not be null.", objRefType, objName));
// proposal
String.format("ObjectReference '%s' of '%s' must not be null.", objRefType, objName));
| 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 List<?> events = [...]
// WITH ASSERT AND EXPLICIT EXTRACTION
assertThat(events).hasSize(1);
String eventType = events.get(0).getEventType();
String details = workbasketHistoryEventMapper.findById(events.get(0).getId()).getDetails();
assertThat(eventType).isEqualTo(WorkbasketHistoryEventType.DISTRIBUTION_TARGET_ADDED.getName());
assertThat(details).contains("\"newValue\":\"WBI:100000000000000000000000000000000001\"");
// WITHOUT ASSERT AND EXPLICIT EXTRACTION
eventType = events.get(0).getEventType();
details = workbasketHistoryEventMapper.findById(events.get(0).getId()).getDetails();
assertThat(eventType).isEqualTo(WorkbasketHistoryEventType.DISTRIBUTION_TARGET_ADDED.getName());
assertThat(details).contains("\"newValue\":\"WBI:100000000000000000000000000000000001\"");
// USING EXTRACTING INSTEAD OF EXPLICIT EXTRACTION
assertThat(events)
.extracting(WorkbasketHistoryEvent::getEventType)
.containsExactly(WorkbasketHistoryEventType.DISTRIBUTION_TARGET_ADDED.getName());
assertThat(events)
.extracting(WorkbasketHistoryEvent::getId)
.extracting(workbasketHistoryEventMapper::findById)
.extracting(WorkbasketHistoryEvent::getDetails)
.containsExactly("\"newValue\":\"WBI:100000000000000000000000000000000002\"");
| We want to use the third option ( | |
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 | 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. | 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. | 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 /wiki/spaces/TAS/pages/968163354. Should we move the testing rules to the child page? |
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? | 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. assertThat(response.getBody()).isNotNull(); assertThat(response.getBody().getReceived()).isEqualTo(expected); // OR TaskRepresentationModel updatedTask = responseUpdate.getBody(); assertThat(updatedTask) .isNotNull() .extracting(TaskRepresentationModel::getReceived) .isEqualTo(expected); // OR assertThat(responseUpdate.getBody()) .isNotNull() .extracting(TaskRepresentationModel::getReceived) .isEqualTo(expected); | Discussion resulted in no guidelines since readability depends on the use case. | |
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? | 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. |
| |
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. Now:  Possible solution:  | 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. 
| 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:  | 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? | 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? | SqlProvider make working easier but we don't necessarily need them everywhere.
| |
Testnaming pattern for simple tests | Keep tests name short for simple tests  | In case it is not possible to name the test with “when” or “for”, it is okay to only write | |
LOMBOK | It’s time to introduce lombok. Please visit https://projectlombok.org/ and look the 4 Minute Video.
|
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!  | PR wird integriert. Weitere Refactorings passieren ohne Ticket in der Zukunft |
