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

Changing created to modified time #6234

Merged

Conversation

@Iamshankhadeep
Copy link
Contributor

Iamshankhadeep commented Oct 2, 2019

fixes #6155

Copy link
Member

dojutsu-user left a comment

Looks like a good first approach.
Few changes are required.

@@ -160,7 +160,7 @@ def record_search_query(project_slug, version_slug, query, total_results, time_s
project__slug=project_slug,
version__slug=version_slug,
created__gte=before_10_sec,

This comment has been minimized.

Copy link
@dojutsu-user

dojutsu-user Oct 2, 2019

Member

I think this should also be changed to modified__gte=before_10_sec.

@@ -210,5 +210,5 @@ def record_search_query(project_slug, version_slug, query, total_results, time_s
version=version,
query=query,
)
obj.created = time
obj.modified = time

This comment has been minimized.

Copy link
@dojutsu-user

dojutsu-user Oct 2, 2019

Member

This line should be removed.
modified field gets updated automatically.

Also, this can be simplified to

SearchQuery.objects.create(
    project=project,
    version=version,
    query=query,
)
@dojutsu-user

This comment has been minimized.

Copy link
Member

dojutsu-user commented Oct 2, 2019

Travis error seems unrelated.

@Iamshankhadeep

This comment has been minimized.

Copy link
Contributor Author

Iamshankhadeep commented Oct 2, 2019

Travis error seems unrelated

Same. The testing was successful on my system, that's why I opened the pull request.

@@ -210,5 +210,3 @@ def record_search_query(project_slug, version_slug, query, total_results, time_s
version=version,
query=query,
)

This comment has been minimized.

Copy link
@dojutsu-user

dojutsu-user Oct 2, 2019

Member

We don't need the obj anymore.

This comment has been minimized.

Copy link
@Iamshankhadeep

Iamshankhadeep Oct 2, 2019

Author Contributor

but SearchQuery.objects.create this thing is needed right?
okay let me update the PR.

This comment has been minimized.

Copy link
@dojutsu-user

dojutsu-user Oct 2, 2019

Member

but SearchQuery.objects.create this thing is needed right?

yes

Copy link
Member

dojutsu-user left a comment

Looks good.
Thanks @Iamshankhadeep 🎉 🎉

@dojutsu-user dojutsu-user requested a review from readthedocs/core Oct 2, 2019
@Iamshankhadeep

This comment has been minimized.

Copy link
Contributor Author

Iamshankhadeep commented Oct 2, 2019

@dojutsu-user why travis is failing, do you have any Idea?

@stsewd

This comment has been minimized.

Copy link
Member

stsewd commented Oct 2, 2019

@Iamshankhadeep test on travis are fixed on #6233 (after that you should merge master into your branch to run the tests again)

@Iamshankhadeep

This comment has been minimized.

Copy link
Contributor Author

Iamshankhadeep commented Oct 2, 2019

@stsewd I got it.

@stsewd

This comment has been minimized.

Copy link
Member

stsewd commented Oct 2, 2019

@Iamshankhadeep you can merge master into this branch now

@stsewd

This comment has been minimized.

Copy link
Member

stsewd commented Oct 2, 2019

@dojutsu-user shouldn't this be removed too?

partial_query.created = time

@humitos

This comment has been minimized.

Copy link
Member

humitos commented Oct 9, 2019

This looks ready to be merged. The only thing missing is to delete what @stsewd mentioned in the previous comment.

@Iamshankhadeep

This comment has been minimized.

Copy link
Contributor Author

Iamshankhadeep commented Oct 11, 2019

@humitos do I need to delete the thing that @stsewd has asked and update the PR?

@dojutsu-user

This comment has been minimized.

Copy link
Member

dojutsu-user commented Oct 13, 2019

I discussed this with @stsewd. Waiting for his review here.

@humitos humitos requested a review from stsewd Oct 14, 2019
@stsewd

This comment has been minimized.

Copy link
Member

stsewd commented Oct 14, 2019

@Iamshankhadeep yes, you still need to remove that

@stsewd

This comment has been minimized.

Copy link
Member

stsewd commented Oct 14, 2019

Just for clarification, what we discuss with @dojutsu-user is about using the celery-created-time (which probably isn't really important to have that precision vs the complexity of keeping track of the exact time) vs using the real-created-time (current implementation) of the query.

In the later case we don't need the time arg on the task anymore (but this can be done in another PR).

@Iamshankhadeep

This comment has been minimized.

Copy link
Contributor Author

Iamshankhadeep commented Oct 14, 2019

So if we use real-created-time then we wont be needing time arg anymore?

@stsewd
stsewd approved these changes Oct 17, 2019
@stsewd

This comment has been minimized.

Copy link
Member

stsewd commented Oct 17, 2019

So if we use real-created-time then we wont be needing time arg anymore?

We can do that in another PR, or just start testing with this change for now

@Iamshankhadeep

This comment has been minimized.

Copy link
Contributor Author

Iamshankhadeep commented Nov 3, 2019

@stsewd why is it not merged?

@stsewd

This comment has been minimized.

Copy link
Member

stsewd commented Nov 4, 2019

@Iamshankhadeep all good, just waiting for an additional review, but we can just merge it.

@stsewd stsewd merged commit aa9cc9f into readthedocs:master Nov 4, 2019
2 checks passed
2 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.