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

exclude deleted annotations from csv download #310

Merged
merged 2 commits into from
Dec 11, 2016

Conversation

cofiem
Copy link
Contributor

@cofiem cofiem commented Dec 4, 2016

see #302

@coveralls
Copy link

coveralls commented Dec 4, 2016

Coverage Status

Coverage increased (+0.005%) to 85.12% when pulling b6b4808 on fix-exclude-deleted-annotations-from-csv-download into 336df7c on develop.

@atruskie
Copy link
Member

atruskie commented Dec 4, 2016

Looks good.

My only question: Does doing the deleted check like I did in https://github.com/QutBioacoustics/baw-server/blob/b6b48089a1f790a3d2874fe4d8137dcc0d4f0d0b/app/models/analysis_jobs_item.rb#L159 offer any real advantages?

@cofiem
Copy link
Contributor Author

cofiem commented Dec 4, 2016

Well, firstly, the comment "cast back to active record relation" is misleading - it isn't a cast, it's using the ActiveRecord from with the Arel query as the query.

I think you need line 160. Without line 160 (query.joins(:audio_recording)), you wouldn't get the ActiveRecord scope, you'd only have the from subquery, so line 160 is necessary, yes.

@atruskie
Copy link
Member

atruskie commented Dec 4, 2016

I meant, for using in this fix... All the default scopes should be used, if they were this bug (#302) wouldn't have happened...

@cofiem
Copy link
Contributor Author

cofiem commented Dec 4, 2016

Oh, righto. I'm not sure. The Arel query might be too complex to be able to make use of ActiveRecord for parts of it. It's a good thought, tho.

@JessieLOliver
Copy link

Hi guys,
I am soooo excited this is happening. One thing I am wondering, can we still see deleted things if we want too? I was envisioning having a way to filter them out/sort, rather then deleting them all together. This isn't critical, but would be helpful to know if one person is changing what another has done.

@atruskie
Copy link
Member

atruskie commented Dec 5, 2016

@JessCappadonna this changeset will remain scoped to fixing the bug only (many small changes increase the speed at which we can ship fixes), however, I've taken your comment and integrated it into ideas we have discussed previously. You can comment and review here: #311

@atruskie
Copy link
Member

atruskie commented Dec 5, 2016

This is ready to merge.

I have an idea to improve it. If I find the time before deploy I will experiment, otherwise, I'll merge.

@coveralls
Copy link

coveralls commented Dec 11, 2016

Coverage Status

Coverage increased (+0.02%) to 85.137% when pulling 67ff093 on fix-exclude-deleted-annotations-from-csv-download into 336df7c on develop.

@atruskie atruskie merged commit 10f58b4 into develop Dec 11, 2016
@atruskie atruskie deleted the fix-exclude-deleted-annotations-from-csv-download branch June 4, 2020 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants