Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix docs multiple nested and multiple methods #5334

Merged
merged 3 commits into from Aug 18, 2017

Conversation

woile
Copy link
Contributor

@woile woile commented Aug 15, 2017

Description

Multiple nested resources with multiple methods were not displayed. Also
detail_routes and list_routes with multiple methods. This PR attempts to fix it.
I created a template tag, that recursively finds the links inside a given section. What do you guys think?
I am not sure if the format I've given to the generated links it's okay, but it's a start.

Issues related:
#5324
#4391
#4965

Results

Default actions

users: {
    list()
    create()
    read(id)
    update(id)
}

screenshot from 2017-08-15 16-18-56


Single action

users: {
    list([page])
}

screenshot from 2017-08-15 16-19-30


Single detail_route url

users: {
    list()
    create()
    read(id)
    update(id)
    farmers(id) <--
}

screenshot from 2017-08-15 16-20-46


More than one detail_route url

users: {
    farmers: {
        read(id) <--
        update(id) <--
    }
    list()
    create()
    read(id)
    update(id)
}

screenshot from 2017-08-15 16-22-17


Nested routes

users: {
        animals: {
            dogs: {
                list(user_id, [page]) <--
                create(user_id, email, content, created) <--
            }
            list(user_id, [page]) <--
            create(user_id, email, content, created) <--
        }
        farmers: {
            read(id)
            update(id)
        }
        list()
        create()
        read(id)
        update(id)
    }

screenshot from 2017-08-15 16-23-16


Multiple resources with nested schema

users: {
    animals: {
        dogs: {
            list(user_id, [page])
            create(user_id, email, content, created)
        }
        list(user_id, [page])
        create(user_id, email, content, created)
    }
    farmers: {
        read(id)
        update(id)
    }
    list()
    create()
    read(id)
    update(id)
}
vets: {  <--
    animals: {
        dogs: {
            list(user_id, [page])
            create(user_id, email, content, created) 
        }
    }
}

screenshot from 2017-08-15 16-24-25


Multiple list_route url

users: {
    farmers: {
        read(id)
        update(id)
    }
    houses: {
        read() <--
        create() <--
    }
    list()
    create()
    read(id)
    update(id)
}

screenshot from 2017-08-15 16-31-20


Single list_route url (houses)

users: {
    farmers: {
        read(id)
        update(id)
    }
    list()
    create()
    houses() <--
    read(id)
    update(id)
}

screenshot from 2017-08-15 16-32-25

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 15, 2017

This is looking good. Thanks!

I'll review fully this week.

"""
Recursively find every link in a schema, even nested.
"""
NESTED_FORMAT = '%s > %s'
Copy link
Collaborator

@carltongibson carltongibson Aug 16, 2017

Choose a reason for hiding this comment

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

Is this going to be valid when used as an HTML id attribute?

Copy link
Contributor Author

@woile woile Aug 16, 2017

Choose a reason for hiding this comment

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

works fine in firefox and chrome under debian x64

e.g:
http://localhost:8000/docs/#users-animals > dogs > list

also clicking the links on the sidebar work.

Copy link
Collaborator

@carltongibson carltongibson Aug 16, 2017

Choose a reason for hiding this comment

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

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 16, 2017

@woile I think this is great! 💃🏼

I like the Vets > animals > dogs > list example. It suggests the CSS is robust
enough. Flat is better than nested: "If you need more nesting than that you'll
need to modify the CSS for yourself." (This is nice limit to draw.)

Tests: The examples you've give seem to cover the core cases. Can you turn them into
unit tests?

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 16, 2017

My only question is (inline above) about the JavaScript data-binding — does that break? (I found it did working around this in a worse way (comment) in #5077)

If so returning an idnested-formated-string and then using another template tag for display might be enough...

What do you think?

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 16, 2017

HTML 5 Spec says of ids:

The value must not contain any space characters.

I'm sure it'll work for some clients but break in others.

Not conclusive but my mail client makes a mess of it:

screenshot 2017-08-16 21 41 39

I think we should adjust this for robustness sake. 🙂

@woile
Copy link
Contributor Author

woile commented Aug 16, 2017

@carltongibson for the ID's maybe we could use the slugify from django which will convert spaces to hiphens. That'd be a solution to deal with the id related problems, right?

I'll try to make the unittests tomorrow.

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 17, 2017

I'll try to make the unittests tomorrow.

Great. Thanks!

I think tests at the schema_links function level will be fine — i.e. just passing a simplified schema document and checking the result; we should be able to trust the whole register.filter and rendering steps, which aren't so nice to test anyway.

@woile
Copy link
Contributor Author

woile commented Aug 17, 2017

@carltongibson I fixed the issues you suggested, tell me what you think.

The generated ID for links now looks like this:
http://localhost:8000/docs/#users-objects-list

And the interaction modal works fine (note: I added a normalize function over the data-key in the form). The good thing about having spaces in the link is that I can split them without worrying because I know a url won't include one.

I'll be working on the tests now.

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 17, 2017

@woile — Looking AWESOME! Thanks for the effort!!!

(If we can get the tests in place, I think we can get this in.)

@woile
Copy link
Contributor Author

woile commented Aug 17, 2017

@carltongibson I've added the tests, tell me what you think. They are a bit long, but the cases are long as well :P

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 18, 2017

@woile The test cases look great. I'll give this a final review today.

Thank you for you effort!

@carltongibson carltongibson added this to the 3.6.4 Release milestone Aug 18, 2017
@carltongibson
Copy link
Collaborator

carltongibson commented Aug 18, 2017

@woile This is good work: it passes my manual testing. 🙂 I'm going to pull this in. Thanks again!

@carltongibson carltongibson merged commit 4d5e846 into encode:master Aug 18, 2017
1 check passed
@woile
Copy link
Contributor Author

woile commented Aug 18, 2017

@carltongibson you are welcome, no problem at all! Glad to help

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

Successfully merging this pull request may close these issues.

None yet

2 participants