Closed sfount closed 8 years ago
Classic "needs tests" alert. :). I'll give you a code review before noon.
Cheers @jniles. I'll be making a few changes today but nothing too significant.
As I've let this pull request span over such a long time I wasn't able to guarantee all of the changes made are good!
No rush on the code review but I'll aim to merge changes from the current development branch to make your life easier.
@sfount, sounds good. Then maybe I'll schedule it for after work tonight.
Yeah, I figured this will take a little while to get merged in. At bhima, we like to make PRs that change 40+ files at a time. :+1:
(42). To be fair this is an example a larger feature that a subteam would work on and then merge up into development. The primary difference being lack of sub team, tests or incremental merges rebasing.
@sfount, I haven't really given a technical review since massive parts of this are changing and rife with TODOs. There are some comments to help clarify some of the concepts here. A few general thoughts (and correct me if I misunderstood anything) follow:
I worry about using resolve
in our routes for a number of reasons. The benefit is that the controller can be written entirely synchronously, and that is convenient. However, I worry that switching our router, updates to AngularJS's Routing API in the future, or other reasons may cause us much more harm in the long run than it might otherwise. Also, if we ever want to start moving in a component-directives direction, it's unclear how we'll use resolve. For example, I'm no clue how this world work:
.when('/some/route', {
template: '<patient-list>',
resolve : someObject // can I get this into the directive??
});
Obviously, all my concerns are purely theoretical, but I'd be interested in discussing it.
Overall, this represents a lot of hard work, and is a great step in the right direction. With regards to your future design questions, my perspective is this:
A proposed structure for integrating PDF document reports into BHIMA. This pull request also proposes a framework for the conceptually complex issue of reporting.
Primarily an Income vs. Expense report document was developed allowing the user to compare the variance on multiple fiscal years as well as demonstrate the report structure.
The proposed report structure is as follows:
Grouping all of BHIMAs reports into these categories will simplify both UI/UX for users as well as finalising a schema for future report development. Using the proposed API will also allow report documents to easily be added given the report data. In this pull request this is demonstrated with the sale records page.
There are a number of design decisions that can be discussed and altered before the pull request is merged with the development branch.
Report Document Integration
General Report Architecture
Reports
Proposed discussions/ implementation before merged - these can be ignored if any not deemed required
The current build introduces a file in server/models called dev.sql to compliment synt.sql - this isn't intended as the final Schema however it will allow the team to discuss (as we would on a white board) before committing it to the upgrade structure - this can be removed/ changed if it is decided against.