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

Conversation

Projects
None yet
4 participants
@stsewd
Member

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 }}">

This comment has been minimized.

@stsewd

stsewd Mar 30, 2018

Member

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

This comment has been minimized.

Member

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?

@humitos

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

This comment has been minimized.

Member

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.

def get_docs_url(self, version_slug=None, lang_slug=None, private=None):
"""
Return a URL for the docs.
Always use http for now, to avoid content warnings.
"""
return resolve(project=self, version_slug=version_slug, language=lang_slug, private=private)

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

stsewd commented Apr 2, 2018

Is this a problem?

Yes, we are back to the initial issue

@stsewd

This comment has been minimized.

Member

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

This comment has been minimized.

Member

stsewd commented Apr 2, 2018

I just create a new file

@humitos

This comment has been minimized.

Member

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:

return reverse(
'project_version_detail',
kwargs={
'project_slug': self.project.slug,
'version_slug': self.slug,
},
)
private = self.privacy_level == PRIVATE

Which generates the link to edit the version.

What I am proposing is to call directly this chunk of code,

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

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

@humitos

This is good!

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

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

stsewd added some commits Apr 2, 2018

version_slug=version.slug,
private=private
)
version_slug=version.slug, private=private)

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

I really liked it in the other way 😞

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 17, 2018

@humitos done!

@agjohnson

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

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

agjohnson commented Apr 18, 2018

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

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Apr 19, 2018

Looks great!

@agjohnson agjohnson merged commit 5562c66 into rtfd:master Apr 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stsewd

This comment has been minimized.

Member

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 stsewd:fix-view-docs branch Apr 20, 2018

@humitos

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

stsewd commented Apr 24, 2018

@humitos working on that right now

@agjohnson

This comment has been minimized.

Contributor

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