Fixes #108

[t-abdul-basser]

Fixes #108

[mark-velez]

Any updates regarding this PR? We also have mark-velez/achilles-integration-pr on standby with the accompanying client-side stuff. Not sure if you were waiting on that. We understand the release crunch can be hectic, please let us know if we can do anything to help.

[chrisknoll]

I'll take this one on.

@t-abdul-basser , @mark-velez : This is going to take a bit of validation, but my plan is to invoke the various web service calls, and try to see about comparing output from the REST calls to the exportToJson files we get from Achilles...I'm assuming that there weren't changes in the raw data structures to do this. Do you have any other suggestion to do a 'respectable' amount of verification on this PR?

[chrisknoll]

Hey, @t-abdul-basser , @mark-velez . I'm going through a lot of these changes, and I'm finding that (at least the first 6 or 8) are just formatting changes. And oddly there's some sort of leading space on some of these lines that shouldn't have an indent at all.

Should I call out in a code comment where I'm seeing all these happen, or would you like to comb through these changes and revert the ones that are just indentation/formatting changes?

[chrisknoll]

Sorry, I take it back (somewhat) re: the leading spacing, the diff viewer confused me for a bit. But the point still stands: if you guys could look at the files changed tab above, and just scroll through the changes (that's how I'm going through the changes) you'll notice that there's just pages and pages of red followed by pages and pages of green that I think is just copies of the code, but different spacing/indentation. Making it much harder to digest what's really changing in those files (if anything at all).

So don't mean to be a snob about this, but i'd really appreciate the cleanup.

-Chris

[mark-velez]

Thanks for getting back so quickly, @chrisknoll. The structure of the output from the REST calls will be very different from the exportToJson output. They will more closely resemble that of analogous calls to the cohort results service (i.e. CohortResultsService). It may be more useful to compare to those or to test with the Atlas branch I refer to above (but I know there are limitations either way). This is part of our effort to eliminate redundant server- and client-side code and instead reuse components that are doing awfully similar things in achilles/heracles. We discussed this in detail during the 12/15 Architecture Workgroup meeting (see slides).

Regarding the formatting changes- I may be to blame for much of them; I understand this problem and typically create explicit formatting-only commits, but it seems to me that this gets obscured by the squash workflow. How do you recommend we handle reformatting changes for future reference? Please pardon my ignorance on the matter.

I think you're right though that there are reformats and duplicates that shouldn't be here. We'll look at it, clean it up and report back. In the meantime, it may still be useful to carry out functional testing as I described above. Thanks again!

[chrisknoll]

I'm going to hold off on reviewing until after all cleanup is made in case I do verify something and then a subsequent cleanup might break something.

Re: squashing: squashing just folds multiple commits into a single commit. If the diff for the entire PR says that 500 lines are removed and the exact same (but reformatted) lines are inserted back, squashing doesn't do anything here.

What you need to do is:

1. Use the files changed tab above
2. Scroll through the page,and identify blocks of code that are just reformatted but not changed.
3. Use your local GIT client to look at the previous commit for that file (this should be easy be cause there's only 2 commits in the entire PR, so finding the previous commit should be straightforward.
4. Copy the entire block from the prior commit into your local file, and commit. This will make it look like that block of code hasn't changed. Just make sure you don't reformat again after you copy the prior :)

If you do this all in one commit, they'rell just be 1 additional commit added to this PR (called something like 'reverted code format changes'). When the squash and merge is done on the PR, all the commits here will be pushed into one commit with all the comments of all the commits put into a single commit message. You can even do it across multiple commits, whatever is easier for you.

-Chris

[t-abdul-basser]

I will look at these formatting issues and address them. Thank you for bringing this to our attention @chrisknoll.

I would second @mark-velez: I would suggest that we move forward with review (e.g. review of substantive changes) while I address these formatting issues.

Thanks.

[chrisknoll]

Could we move away from using the webapi path starting with the source key? This has always been something that's been counter-intuitive....most of the time you have WebAPI/{service}/{sub-route path...} but other cases you start with a source key.

Consider if instead we have the cdmresults service directly off of WebAPI. We can choose to handle this route by returning a response that returns the list of sources that have cdmresults generated, or we can not have a handler for this 'default service route' (requiring the source like we do today, but in a form of /{servicename}/{source} format. But if we do implement the service route, this default route could be the thing that drives the datasource selection dropdown (ie: the datasources.json in the old achilles web). Then using one of the datasources will take you to routes beginning with /cdmresults/{sourcekey}/{path to specific reports}.

[t-abdul-basser]

@chrisknoll I agree. I suggest that we do this later however, outside the scope of this PR.

[chrisknoll]

I think the scope of the PR is to introduce the new endpoints. Shouldn't we add the endpoints with the correct format rather that release it incorrect and then have to deal with backwards compatibility later?

[chrisknoll]

@mark-velez , I took a look at the fork of atlas, but couldn't create any issues in your fork to discuss some of the things I'm seeing. I'm not sure if forked repos have their own issues and I just can't do it becuase it's restricted some way? Not sure, but I'll try to reach out to you via email about what I'm seeing.

[mark-velez]

@chrisknoll I, well, perhaps we just learned that you have to explicitly enable the issues feature on forks. You should be able to submit issues on my Atlas fork now. Please note that @fdefalco asked me to look into rebasing from the branch v2-integration-candidate-pr. I just completed that on my branch achilles-integration-pr-1.

[chrisknoll]

Excellent, thanks, I see it now.

[t-abdul-basser]

Yes, the PR is intended to introduce the new endpoints. However, there were endpoints that existed prior to my introduction of the achilles report-related endpoints so this change would essentially be an cdmresults-level change i.e. one that effects the course-grained service. While that is not conceptually difficult, since I did not introduce this format (it appears that @fdefalco might have done so) and this would change client side invocations in Atlas I would still prefer to undertake the refactoring later.

[chrisknoll]

Unfortunately, those existing endpoints where introduced in Oct of '15 when we didn't have the benefit of reviewing and experience that we do today.

It feels like the path forward is to accept the PR as as and try to make changes later. So I'll approve the PR and create issues of things I see separately.

[t-abdul-basser]

I understand fully. I see that you have open #166. Thanks for taking time to do that. Pleae assign to me and I will handle that on next pass.

[chrisknoll]

I'll handle #166, I reviewed where the changes also need to occur on the Atlas side so I can coordinate both changes.