Skip to content
This repository has been archived by the owner on Feb 22, 2020. It is now read-only.

New API endpoint to fetch dataset directory #81

Merged
merged 7 commits into from Sep 8, 2017

Conversation

rahit
Copy link
Contributor

@rahit rahit commented Aug 27, 2017

New API for direcotory listing in homepage as described in #77

Description

Introducing new API endpoint which will be used to retrieve images from data directory.

Reference to official issue

See issue #77

How Has This Been Tested?

This modification will effect homepage initial dataload. Implementation of the API is yet to be made.

Updates of Acceptance Criteria

  • Update routes
  • Declare view (Not implementation)
  • Update index.html API call

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

.gitignore Outdated
# ignore Sphinx apidoc
_apidoc_*
_apidoc_*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@@ -9,6 +9,7 @@
CandidateViewSet,
NoduleViewSet,
ImageSeriesViewSet,
ImageSrcApiView
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a trailing comma to avoid VCS noise for the next addition! :)

@lamby
Copy link
Contributor

lamby commented Aug 28, 2017

Hey, thanks for this! Please rebase and — whilst you're at it — address those tiny niggling things I brought up :)

@rahit
Copy link
Contributor Author

rahit commented Aug 29, 2017

@lamby Resolved conflicts and modified as per your suggestion. I have mistakenly used master of forked repo for the pull request. I will do it in new branch for future changes. :)

Copy link
Contributor

@isms isms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great work! I think we're headed in the right direction.

A few requested changes and one larger question.

.gitignore Outdated


# ignore Sphinx apidoc
_apidoc_*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change - see #80 (in particular https://github.com/concept-to-clinic/concept-to-clinic/pull/80/files)

@@ -9,6 +9,7 @@
CandidateViewSet,
NoduleViewSet,
ImageSeriesViewSet,
ImageSrcApiView,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to ImageAvailableApiView or something more descriptive?

@@ -19,5 +20,6 @@

urlpatterns = [
url(r'^', include(router.urls)),
url(r'^image_src/$', ImageSrcApiView.as_view()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, how about ^images/available$?

"""
Return a list of files and folders in dataset
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to implement or leave this stubbed out? Stub is acceptable if that's the plan, we'll just create another issue for actually implement this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can create a new issue for this.

@lamby
Copy link
Contributor

lamby commented Aug 31, 2017

Hi @rahit
Do you need any help or clarification with the review feedback? :)

@rahit
Copy link
Contributor Author

rahit commented Aug 31, 2017

Thank you @lamby for the suggestions. I have not get the chance to work on the modifications yet. I would like to let you know that your feedback is clear to me and I will get back asap.

@lamby
Copy link
Contributor

lamby commented Sep 1, 2017

Thank you @lamby for the suggestions. I have not get the chance to work on
the modifications yet. I would like to let you know that your feedback is clear to me
and I will get back asap.

Brilliant stuff, looking forward to it :)

Copy link
Contributor

@lamby lamby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good :)

@@ -100,8 +100,8 @@ <h3 class="card-title">{{ selected.patient_id }}</h3>
methods: {
fetchData: function() {
var vm = this;
this.$http.get('/api/images/').then(
function(response) {
this.$http.get('/api/images/available').then(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think this should use {% url … %} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whole <script> is under {% verbatim %}. Even if verbatim is omitted from script section, is it possible to use {% url %} inside Vue ?

"""
Return a list of files and folders in dataset
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, should this return at least some kind of empty result? :)

@rahit rahit force-pushed the master branch 2 times, most recently from f2fc786 to ed3e55f Compare September 5, 2017 08:42
@lamby
Copy link
Contributor

lamby commented Sep 6, 2017

Hey all, who is this blocking on? Whilst we are assigned to @rahit here, that may not be accurate AFICT; please update if this is wrong! — lamby

@rahit
Copy link
Contributor Author

rahit commented Sep 6, 2017

@lamby let me know if there is anything that should be done for this pull request.

@lamby
Copy link
Contributor

lamby commented Sep 7, 2017

@rahit Can you rebase against master; currently conflicting there :)

@rahit
Copy link
Contributor Author

rahit commented Sep 7, 2017

@lamby done

@lamby lamby merged commit c3d5ac7 into drivendataorg:master Sep 8, 2017
@lamby
Copy link
Contributor

lamby commented Sep 8, 2017

Thanks, merged @rahit
:)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants