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 view docs link #3882

Merged
merged 4 commits into from Apr 19, 2018
Merged

Fix view docs link #3882

merged 4 commits into from Apr 19, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 30, 2018

Fix #3879

So, to be able to fetch the resulting url without a hardcoding solution I had to add a new field to the API response.

What is missing?

  • Rebuild the js
  • Maybe update tests
@@ -56,7 +56,7 @@

<div data-bind="visible: finished() && success()">
<li>
<a class="" href="{{ build.version.get_absolute_url }}">
<a data-bind="attr: {href: docs_url}" href="{{ build.version.get_absolute_url }}">
Copy link
Member Author

@stsewd stsewd Mar 30, 2018

This data-bind was taken from http://knockoutjs.com/documentation/attr-binding.html.

Also I left the href in case someone has the js disable, but there is a condition first, and all the builds will be no show if someone doesn't have the js activated, so this can be deleted I guess.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 30, 2018

Also, this time I tested it with a new project p:

mmm, all tests passed. Maybe we should add a test for the API response here?

Copy link
Member

@humitos humitos left a comment

Good this is fixed now and that you handle a knockout solution! It was hard to me at the beggining :)

Although this solve the issue, I'm not sure that I like the solution. It seems kind of overkill to me and also the response from that endpoint is not always accurate: the first time we hit that enpoint we will have a docs_url field which is a "lie", I mean, it doesn't point to the docs URL but to the edit version URL.

So, to me, as I said, I think this solves this particular issue but I think we need a general solution that do not potentially affect other uses of this endpoint.

I don't have a better solution from the top of my head but we should think in a better one. Maybe a simple templatetag that calls

self.project.get_docs_url(
    version_slug=self.slug, private=private)

is a better/simpler solution.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 2, 2018

@humitos there is a get_docs_url method, but the result is from a custom resolver, which handles domains, subprojects, unique versions, etc.

https://github.com/rtfd/readthedocs.org/blob/393e31ad3a9aafee297df64f1a654ffcda7ef04a/readthedocs/projects/models.py#L368-L374

The first calls to the endpoint wouldn't make sense, but this should only be used when the build is completed and marked as successful.

@humitos
Copy link
Member

@humitos humitos commented Apr 2, 2018

there is a get_docs_url method, but the result is from a custom resolver, which handles domains, subprojects, unique versions, etc

Is this a problem?

The first calls to the endpoint wouldn't make sense, but this should only be used when the build is completed and marked as successful.

This is called every 2s on the build page, so the only call that will make sense is the latest one. But anyway, what I want to avoid is having a endpoint that return non accurate data.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 2, 2018

Is this a problem?

Yes, we are back to the initial issue

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 2, 2018

@humitos I did a quick test with a custom templatetag, it works. Should I create a new file or put it on projects_tags? This is the definition def get_docs_url(build):

@stsewd stsewd force-pushed the fix-view-docs branch from e9d3ee1 to 712e1c4 Apr 2, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 2, 2018

I just create a new file

@humitos
Copy link
Member

@humitos humitos commented Apr 2, 2018

Yes, we are back to the initial issue

No. The original issue was that this chunk of code was called:

https://github.com/rtfd/readthedocs.org/blob/393e31ad3a9aafee297df64f1a654ffcda7ef04a/readthedocs/builds/models.py#L156-L163

Which generates the link to edit the version.

What I am proposing is to call directly this chunk of code, https://github.com/rtfd/readthedocs.org/blob/393e31ad3a9aafee297df64f1a654ffcda7ef04a/readthedocs/builds/models.py#L164-L166

that no matter if the Version is built/uploaded or not the link is generated properly. Since, it will be shown only in a success build, there is no problem of getting 404.

@agjohnson agjohnson added this to the 2.4 milestone Apr 11, 2018
Copy link
Member

@humitos humitos left a comment

This is good!

Can you just use pre commit in the new file you created?

After that, it's ready to merge to me.

Add new templatetag
version_slug=version.slug,
private=private
)
version_slug=version.slug, private=private)
Copy link
Member Author

@stsewd stsewd Apr 17, 2018

I really liked it in the other way 😞

Copy link
Member

@humitos humitos Apr 17, 2018

You just need to add a trailing comma in the private line.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 17, 2018

@humitos done!

Copy link
Contributor

@agjohnson agjohnson left a comment

I think a more correct solution is to get this information from the API return and use this in the knockout view.

The problem this introduces is having a third or fourth code location where we have similar logic. So, i'm -1 on splitting this up further.

@humitos
Copy link
Member

@humitos humitos commented Apr 17, 2018

Ups... I told him to use a template tag instead of a call to the API as he has done before and I mentioned a problem that the first solution using the API had at:

#3882 (review)

Querying to the API while building return a docs_url that wasn't accurate. So, probably that should be none or similar.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 18, 2018

Yeah, we probably need to think more about what we're returning from the api here.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 19, 2018

Looks great!

@agjohnson agjohnson merged commit 5562c66 into readthedocs:master Apr 19, 2018
1 check passed
@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 19, 2018

@agjohnson actually we need to generate the minified js here, or the build page would break

@stsewd stsewd deleted the fix-view-docs branch Apr 20, 2018
@humitos
Copy link
Member

@humitos humitos commented Apr 24, 2018

@stsewd can you do that minified js?

Also, I think it would be good to have a test for this new attribute in the API response if we don't have it yet.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 24, 2018

@humitos Actually I was writing the tests, and then I realize that the branch got merged 😁

The minified js was added to the relcorp branch 145390a, I was hoping that it was on the master branch too.

@humitos
Copy link
Member

@humitos humitos commented Apr 24, 2018

@humitos Actually I was writing the tests, and then I realize that the branch got merged

Can you write the test cases in a different PR?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 24, 2018

@humitos working on that right now

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 25, 2018

Yes, i bundled everything up at deploy. Our release process should also likely be building assets, but we should be forcing PRs that update JS to also update assets. Maybe a good use for probot later.

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

Successfully merging this pull request may close these issues.

None yet

4 participants