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

Redirect to build detail post manual build #4622

Merged

Conversation

@ba11b0y
Copy link
Contributor

@ba11b0y ba11b0y commented Sep 8, 2018

Fixes #4359.
Also sorted imports on running pre-commit hook and some minor modifications to comply with flake8.

Copy link
Member

@stsewd stsewd left a comment

Thanks for your contribution!



class BuildList(BuildBase, BuildTriggerMixin, ListView):

def get_context_data(self, **kwargs):
context = super(BuildList, self).get_context_data(**kwargs)

active_builds = self.get_queryset().exclude(state="finished").values('id')
active_builds = self.get_queryset().exclude(state='finished'
).values('id')
Copy link
Member

@stsewd stsewd Sep 10, 2018

This line is wrapped in a weird way, If this is from pre-commit I think we can just format this manually. /cc @humitos

Copy link
Contributor Author

@ba11b0y ba11b0y Sep 10, 2018

Yes it was the result of the pre-commit hook.

Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

yup, this is an indenting pattern we don't support.

Copy link
Member

@humitos humitos Oct 1, 2018

yapf sometime fails with weird lines.

I check them manually sometimes in a very fast way and decide to not include them when it happens.

Copy link
Member

@humitos humitos Oct 1, 2018

Please, fix this yapf indent mistake.

return HttpResponseRedirect(reverse('builds_project_list', args=[project.slug]))
build_pk = project.builds.first().pk
return HttpResponseRedirect(
reverse('builds_detail', args=[project.slug, build_pk])
Copy link
Member

@stsewd stsewd Sep 10, 2018

We may want a test for this, I'll be testing this manually later

Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

I think there is a better way to get the pk of the build we just created. project.builds.first() is not explicit enough i feel. trigger_build calls prepare_build, which is where the build object is created. Perhaps we already have access to the build pk on the task signature return somehow?

Copy link
Contributor Author

@ba11b0y ba11b0y Sep 24, 2018

I will add a commit. Busy with exams right now.

Copy link
Contributor

@agjohnson agjohnson left a comment

Looks good, +1 on tests for this. Also if we can get an explicit build id, I think that makes for the most guaranteed UX.

return HttpResponseRedirect(reverse('builds_project_list', args=[project.slug]))
build_pk = project.builds.first().pk
return HttpResponseRedirect(
reverse('builds_detail', args=[project.slug, build_pk])
Copy link
Contributor

@agjohnson agjohnson Sep 19, 2018

I think there is a better way to get the pk of the build we just created. project.builds.first() is not explicit enough i feel. trigger_build calls prepare_build, which is where the build object is created. Perhaps we already have access to the build pk on the task signature return somehow?

@ba11b0y
Copy link
Contributor Author

@ba11b0y ba11b0y commented Sep 27, 2018

@agjohnson I am unable to get the result of the task since celery gives a runtime warning when task_always_eager is True.

res = trigger_build(project=project, version=version)
res = AsyncResult(res.task_id)
res.get() # *** RuntimeError: Cannot retrieve result with task_always_eager enabled

A related PR at celery tries to change it to a RuntimeWarning

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 27, 2018

We don't want .get() anyways, as that is blocking. Poking around I guess we don't get the signature back with EagerResult from trigger_build return currently. If we have the signature, we can do:

signature = prepare_build(proj)
build_pk = signature.get('kwargs', {}).get('build_pk')

This might need to be verified, but I don't think we ever use the return from trigger_build. If not, we can either:

  • Make the return the signature from prepare_build
  • Make the return a tuple including the signature from prepare_build

Then we can easily get the build_pk from the signature

@ba11b0y
Copy link
Contributor Author

@ba11b0y ba11b0y commented Oct 1, 2018

@agjohnson I have pushed a commit which returns a tuple now and uses the signature to fetch the build_pk. Should I squash the commits? I am sorry I didn't rebase before commiting.
Also I don't know search tests are failing

@humitos
Copy link
Member

@humitos humitos commented Oct 1, 2018

There is a problem with the PR since it says that ~1500 files were modified :/

Could you please rebase your changes against master and check that it's correct, please?

@ba11b0y ba11b0y force-pushed the fix/redirect-to-build-detail branch from a807a00 to 768779e Oct 1, 2018
@ba11b0y
Copy link
Contributor Author

@ba11b0y ba11b0y commented Oct 1, 2018

@humitos Sorry. Done now. I still don't know why the tests fail here.

Copy link
Member

@humitos humitos left a comment

Thanks for your contribution!

This PR looks good to me.

I'm marking as Request changes because a test case is needed. Once the test is added, I would be ready to merge from my point of view.

Also, I mentioned some small style changes to be considered.

@@ -172,7 +172,7 @@ def trigger_build(project, version=None, record=True, force=False):
# Current project is skipped
return None

return update_docs_task.apply_async()
return (update_docs_task.apply_async(), update_docs_task)
Copy link
Member

@humitos humitos Oct 1, 2018

The docstring needs to be updated accordingly.

Copy link
Contributor Author

@ba11b0y ba11b0y Oct 1, 2018

Yeah. Missed it. Also is the AsyncResult promise necessary? I don't find any use of returning it from the function.

Copy link
Member

@humitos humitos Oct 2, 2018

It's not used, I think.

It returns the async result since it's what it creates and could be useful to extend this from outside.

Copy link
Contributor Author

@ba11b0y ba11b0y Oct 5, 2018

@humitos Should I create a new test or modify the existing tests. Also the existing tests don't actually use any return from trigger_build, so I am a bit confused.

Copy link
Member

@humitos humitos Oct 9, 2018

Modify the test and update the docstring to reflect your changes.

Copy link
Contributor Author

@ba11b0y ba11b0y Oct 17, 2018

@humitos Sorry but I still can't figure out what to modify in test_core_utils or in other tests since they do not utilize the build_pk

Copy link
Contributor Author

@ba11b0y ba11b0y Oct 22, 2018

@humitos @agjohnson Ping. Need some help with the tests.

I still can't figure out what to modify in test_core_utils or in other tests since they do not utilize the build_pk.

readthedocs/builds/views.py Outdated Show resolved Hide resolved


class BuildList(BuildBase, BuildTriggerMixin, ListView):

def get_context_data(self, **kwargs):
context = super(BuildList, self).get_context_data(**kwargs)

active_builds = self.get_queryset().exclude(state="finished").values('id')
active_builds = self.get_queryset().exclude(state='finished'
).values('id')
Copy link
Member

@humitos humitos Oct 1, 2018

Please, fix this yapf indent mistake.

@ba11b0y ba11b0y force-pushed the fix/redirect-to-build-detail branch from 5729816 to bdc332b Oct 1, 2018
@ba11b0y ba11b0y force-pushed the fix/redirect-to-build-detail branch from 94e13af to 754b60d Oct 12, 2018
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 2, 2018

I've added a test here. I think it should be 💯

@ericholscher ericholscher force-pushed the fix/redirect-to-build-detail branch from 51226ed to c972fb2 Nov 2, 2018
@codecov
Copy link

@codecov codecov bot commented Nov 2, 2018

Codecov Report

Merging #4622 into master will increase coverage by 0.05%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #4622      +/-   ##
==========================================
+ Coverage   76.42%   76.47%   +0.05%     
==========================================
  Files         158      158              
  Lines        9992     9992              
  Branches     1262     1262              
==========================================
+ Hits         7636     7641       +5     
+ Misses       2015     2009       -6     
- Partials      341      342       +1
Impacted Files Coverage Δ
readthedocs/builds/views.py 96% <100%> (+10.28%) ⬆️
readthedocs/projects/views/private.py 80.1% <80%> (ø) ⬆️
readthedocs/core/utils/__init__.py 74.73% <85.71%> (-0.27%) ⬇️

@ericholscher ericholscher merged commit 1057273 into readthedocs:master Nov 5, 2018
3 checks passed
@humitos
Copy link
Member

@humitos humitos commented Nov 6, 2018

@invinciblycool THANKS!

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

Successfully merging this pull request may close these issues.

None yet

6 participants