Technical Debt for v3

All services

• Standardize utility classes into the util package, NOT utils.
• Turn on disabled tests.
• Audit to make sure all relevant unit tests/automated tests from v2 are covered in v3.
• Add more thorough logging. (A quick review of the reference-data and requisition services suggest that controllers sometimes perform CRUD operations without logging anything.)
• Showing a service's version creates a new Java object. Seems like it should just be a static method, since version info doesn't change over the life of the application.

Auth Service

• Reverse the call from Reference Data to Auth Service.  Having this implies that to use Reference Data, you need to call a specific endpoint on auth
• Remove auth's hardcoded list of services  OLMIS-1498 - Getting issue details... STATUS
• Auth endpoints regarding initiate user password, change password, and forgot password aren't very restful

Stockmanagement Service

• Integration tests depend on a certain amount of demo data to be loaded when running (found integration test error about nodeId not existing when trying to move to new demo data loading approach, which does not load demo data during integration tests)

Fulfillment Service

• Convert to order should be a fulfillment right. It is by warehouse, not by program. 
• Need an integration test for FacilityFtpSettingRepository.findFirstByFacilityId (done: test)
• Order domain object should not use Json annotations, but DTO for serial/deserial
• Remove all hardcoded English strings that get returned to the client  OLMIS-1399 - Getting issue details... STATUS
• Remove extra headers (X-Content-Type-Options, X-XSS-Protection) in RAML (done: review)
• All endpoints should have controller integration tests for different HTTP codes; add JSON schemas (generally a regular JSON for 2XX codes, and errorResponse for 4XX codes)  OLMIS-1499 - Getting issue details... STATUS
• Remove 500 errors from RAML (any endpoint can return it, but we do not have integration tests for these cases)
• Change errorResponse.json to have message and messageKey, not message and description  OLMIS-1399 - Getting issue details... STATUS
• Error handling classes should not log error, but should log debug (these are not actually errors)
• Order.status should not be settable from a DTO.
• Need to redesign to remove Dtos from Order domain object.
• Doesn't make sense for StatusMessage object to be part of Order object. Doesn't seem to be used anywhere and if it's needed, it should be retrieved from the Requisition service.
• Doesn't make sense for StatusChange object to be part of Order object. Only thing that appears to need it is the order report, and that could be filled in the DTO. It should be retrieved from the Requisition service.
• It's unclear what facilityId is used for and why it's necessary, since there is requestingFacilityId, supplyingFacilityId and receivingFacilityId.
• Move all authentication stuff in OrderController and ProofOfDeliveryController to PermissionService.
• OrderController.getAllOrders should be paginated, if we need it. We may not want to have it, and just have everyone use the order search endpoint.
• ExporterBuilder seems strange, as every time you need to convert something to DTO, you have to add another method to it.
• Don't quite understand the need for Storable, since TransferProperties is the only one that uses it. Is this abstraction necessary?
• Don't quite understand the need for OrderSender, since OrderFtpSender is the only one that uses it. Is this abstraction necessary?
• Don't quite understand the need for TransferPropertiesFactory and TransferPropertiesConverter. The code is hard to follow.
• Update README (stale)

Notification Service

• Refactor from-address.  Each client to this service shouldn't have to tell it the from-address of the application.  It should be source, by Notification service, from where it's configured - currently an environment variable
• Refactor how a message is sent, a client should have the responsibility of telling the Notification service the message it wants to send and the forms of that message (by language, by channel, etc).  The notification service should be responsible for finding the address (email, sms, smoke signals) of the user to notify, the user's channel preference (SMS/email/sky writing), and someday it might even have find the user's desired time of day to be notified.
• Make sending notifications async - the client tells the service, the service is responsible for queuing it and then processing the queue as needed (e.g. on load, on channel quotas, retries when channel unavailable, etc)
• Add integration tests with mock smpt server for sending notification https://github.com/OpenLMIS/openlmis-notification/blob/master/src/integration-test/java/org/openlmis/notification/web/NotificationControllerIntegrationTest.java#L49

Requisition Service

• When configuring a requisition template, the endpoint needs to check for valid IDs of program and facility type
• Calculated fields are using Helper classes, not putting all business logic inside the Domain objects (let's ask for a meeting to explain it; Chongsun said the code is hard to follow)
• PermissionService.hasPermission by requisitionId should throw exception if requisition is null (otherwise hasPermission continues without problems even though ID is not valid) Moved into OLMIS-1859
• RequisitionService.convertToOrder should be releasing a requisition in the domain layer (requisition.release(), or requisitionReleaser.release(requisition)), not in the service layer (which just sets status)
• /requisitions/submitted should go away; it can be handled by the /requisitions/search endpoint with a filter
• BooleanResultDto should be refactored to ResultDto<Boolean>
• PermissionServiceTest.hasRight should have more descriptive name
• Remove all hardcoded English strings that get returned to the client
• Remove extra headers (X-Content-Type-Options, X-XSS-Protection) in RAML
• All endpoints should have controller integration tests for different HTTP codes; add JSON schemas (generally a regular JSON for 2XX codes, and errorResponse for 4XX codes)
• Remove 500 errors from RAML (any endpoint can return it, but we do not have integration tests for these cases)
• Change errorResponse.json to have message and messageKey, not message and description
• Error handling classes should not log error, but should log debug (these are not actually errors)
• Stock Adjustment Reasons are supposed to be configured by program but the API endpoints do not specify a program (see OLMIS-381 and compare to Swagger/RAML for /api/stockAdjustmentReasons)
• Update README (stale)
• Remove DTOs from Requisition and RequisitionLineItem (and potentially other) domain objects. Instead, should snapshot the data needed and store it in those domain objects.
• For good OO, Requisition objects should be created with a processing period required (which would also populate the field numberOfMonthsInPeriod)
• Some requisition endpoints, the logic is in the RequisitionController and for some, the logic is in the RequisitionService and it is not clear why one is chosen vs. the other (ex: skip and delete has logic in the service, while submit, authorize, approve is in the controller). This is a problem when trying to create a helper method; then this helper method needs to be implemented in both the controller and service (breaking DRY), but the helper method does not quite fit in being a fully public method
• Controller integration tests are not mocking anything below the controller (not on Spring Boot 1.4, and not using MockBean annotation) this is OLMIS-1142
• Need to create status message controller integration tests once existing integration tests are using a mocked pattern
• RequisitionLineItems can be directly modified, but should be encapsulated inside Requisition logic. There are a lot of public methods in the RequisitionLineItem class.
• Need to create jasper template controller integration tests once existing integration tests are using a mocked pattern
• Requistion template endpoints use RequisitionTemplate domain object, not DTO this is OLMIS-2112

Reference Data Service

• Lot Codes were implemented (OLMIS-2262) with a validator that requires global uniqueness. That logic is probably incorrect. 

  • In LotValidator.java verifyCode() method, it forces each lot to be globally unique. I believe correct behavior is that LotCodes are only unique within a given Trade Items. Definitely two different TradeItems might both have a Lot Code "100", for example. OR perhaps we should have no constraints around uniqueness at all.

• /users/userId/fulfillmentFacilities needs rightId parameter (similar to /users/userId/supervisedFacilities)
• SupplyLine search has search and searchByUUID, which seems like it should be consolidated to one search endpoint.
• Remove all hardcoded English strings that get returned to the client
• Remove extra headers (X-Content-Type-Options, X-XSS-Protection) in RAML
• All endpoints should have controller integration tests for different HTTP codes; add JSON schemas (generally a regular JSON for 2XX codes, and errorResponse for 4XX codes)
• Remove 500 errors from RAML (any endpoint can return it, but we do not have integration tests for these cases)
• Change errorResponse.json to have message and messageKey, not message and description
• Error handling classes should not log error, but should log debug (these are not actually errors)
• Update README (stale)

UI

Ready For Work

Backlog

Platform

Below are platform-level issues. They may impact multiple services or impact the build and tooling. Many of these items were formerly in the "Dev and Build Infrastructure" epic (OLMIS-584); much of that epic was moved into label=TechBacklog, component=Platform, and fixVersion was erased so we could re-prioritize what of these issues are a priority.

Ramp Up

The items in this section are some of the ones noticed while starting to ramp up on OpenLMIS v3 - and thus seeing the project through a newbie's eyes. They aren't necessarily wrong - just things that gave me pause.

Errors

When starting openlmis-ref-distro via "docker-compose up" sans the -d flag, a large number of SQL errors are printed to the console. I was eventually told that they're expected and not symptomatic of a true problem. Because they're the last thing printed to the screen, however, without any subsequent indication that openlmis-ref-distro successfully launched, they lead new users to believe that openlmis-ref-distro has failed to start.

Dated Instructions

The instructions at https://hub.docker.com/r/openlmis/dev/ seem dated. For example, running a build as follows, and as recommended, doesn't work:

docker run -it --rm -p 8080:8080 -v gradlecache:/gradle -v $(pwd):/app openlmis/dev
gradle clean build

The instructions at https://github.com/OpenLMIS/openlmis-ref-distro also seem a little old. They say to access the UI via http://<your ip-address>/public, which doesn't work. Instead, stripping /public from the address seems proper. It would certainly be easy to update the ReadMe... but I'm not sure whether the current behavior is intended.

Given that services are no longer intended to be run independently, openlmis-ref-distro seems to play a prominent role. It doesn't seem to be mentioned in Read The Docs, though. The closest thing is a link to a live-api mentioned in the Style Guide section.

Finally, because http://localhost/api returns a 404 error, it may be helpful to mention somewhere that Swagger is available at:

http://localhost/requisition/docs/
http://localhost/fulfillment/docs/
http://localhost/auth/docs/
http://localhost/referencedata/docs/
etc. 

During our retrospective, the notion of an introductory ramp-up guide came up. I think one would be helpful - and that Read The Docs would be the perfect place for it. As is, we have lots of documentation for a variety of services. It's difficult to consume all at once, though. Readers don't know where to start, and much of it seems repetitive. The ReadMe for OpenLMIS-ref-distro, a project which ties everything together, may be a good place to begin. As mentioned, though, it isn't even mentioned in Read The Docs. 

Read The Docs

Disclaimer: This section describes my first impression of our Read The Docs page. It's therefore just personal opinion - nothing more.

As is, our Read The Docs page is notably unclear. This is an important issue because:

A) The purpose of documentation is to provide clarity - documentation which does the opposite is thus fundamentally flawed. 
B) It may be among the first OpenLMIS assets that developers are exposed to. 

Our landing page, along with the view returned when users click "Overview," doesn't offer any introductory information about the project. In fact, it doesn't present any handwritten information. Although there are links to it, I think our documentation should be much more prominent.

In addition to adding introductory text, the following small changes would make a big difference:

• Either remove the "View Docs" button, given that it's redundant, or rename it "View Current Documentation" and align it with all the other buttons. I prefer the later. An element this fundamental to the website's purpose should be front and center - not relegated way off to the side.
• Remove the "Builds" button, along with section highlighted in the screenshot below. Unless I'm missing something, they're needless distractions to most readers.
  
  

Searching ReadTheDocs.org for "openlmis" returns "No results found. Bummer." Users have to add a space ("open lmis") to find the project. This is a pretty trivial issue. It matters, though, when certain operations (like changing the selected language or clicking the page's banner) redirects folks to Read the Docs' home page.
 

Style Guide

Code

Our Testing Guide suggests that integration test “need to be kept separate from build.” The build task in the referencedata service, though, lists “check” as a dependency, which in turn lists “integrationTest.” Should this be changed?