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

feat: Favorite queries #520

Merged
merged 11 commits into from Jan 21, 2023
Merged

Conversation

lawson89
Copy link
Contributor

Allow users to 'favorite' queries

Issue #119

Issue chrisclark#119

Heart at top of query view allows user to toggle favorite or not
Added Favorites list to top menu
Issue chrisclark#119

fix isort and flake8 issues
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023

Codecov Report

Merging #520 (61ebf5a) into master (b1d3997) will increase coverage by 0.04%.
The diff coverage is 98.41%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage   96.55%   96.60%   +0.04%     
==========================================
  Files          52       54       +2     
  Lines        2238     2300      +62     
==========================================
+ Hits         2161     2222      +61     
- Misses         77       78       +1     
Impacted Files Coverage Δ
explorer/urls.py 100.00% <ø> (ø)
explorer/views/query_favorite.py 95.83% <95.83%> (ø)
explorer/migrations/0011_query_favorites.py 100.00% <100.00%> (ø)
explorer/models.py 98.42% <100.00%> (+0.04%) ⬆️
explorer/tests/test_views.py 100.00% <100.00%> (ø)
explorer/views/__init__.py 100.00% <100.00%> (ø)
explorer/views/utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lawson89
Copy link
Contributor Author

@marksweb let me know if you have any questions or tweaks to this

@marksweb marksweb changed the title Feature/query favorites feat: Favourite queries Jan 13, 2023
Copy link
Collaborator

@marksweb marksweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one where I need to think a little differently to ignore the difference in US spelling 😂
I'll install this as it is to get a feel for it, but it looks good.

There's just a few comments I've left that should improve things a little.

explorer/views/query_favorite.py Outdated Show resolved Hide resolved
explorer/views/query_favorite.py Outdated Show resolved Hide resolved
explorer/views/query_favorite.py Outdated Show resolved Hide resolved
explorer/views/query_favorite.py Outdated Show resolved Hide resolved
@marksweb marksweb changed the title feat: Favourite queries feat: Favorite queries Jan 14, 2023
lawson89 and others added 2 commits January 14, 2023 08:24
Co-authored-by: Mark Walker <theshow@gmail.com>
Issue chrisclark#119

Incorporate review feedback
@marksweb
Copy link
Collaborator

Thanks @lawson89!

lawson89 and others added 2 commits January 14, 2023 21:45
Co-authored-by: Mark Walker <theshow@gmail.com>
Issue chrisclark#119

Make favorite icon red to standout
@lawson89 lawson89 marked this pull request as draft January 16, 2023 12:32
Issue chrisclark#119

Make favorite icon a tag
Reuse on front page of query list
@lawson89
Copy link
Contributor Author

@marksweb please take a look and let me know what you think. I'm not sure tbh why the unit-tests-future-versions failed?

@marksweb
Copy link
Collaborator

@lawson89 will take a look as soon as possible.

That test has failed because Django's main branch has moved on to v5 after they released the 4.2 alpha. And v5 needs a minimum of python 3.10, so I need to update the test suite.

@marksweb
Copy link
Collaborator

@lawson89 I've updated the test suite for django 4.1 alpha and django 5 (main). Could you rebase your branch please? I can't update from here.

@lawson89
Copy link
Contributor Author

@marksweb done. Not sure why precommit fails, flake8 passes for me?

@marksweb
Copy link
Collaborator

@lawson89 It's likely parts of the project that aren't part of this PR.

The CI bot will check everything and there's a good chance I've added pre-commit but not yet ran it against the existing codebase. I'll take a look and there might be another update to master that needs rebasing soon.

@marksweb
Copy link
Collaborator

@lawson89 Ok I've updated master with pre-commit having fixed everything in it's checks. Rebase your branch and all will pass 👍

Copy link
Collaborator

@marksweb marksweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. Very nearly ready I think.

I've just added a few comments that I think improve things and complete the feature.

explorer/templates/explorer/query_list.html Outdated Show resolved Hide resolved
explorer/templates/explorer/query_list.html Outdated Show resolved Hide resolved
explorer/templates/explorer/query_favorite_button.html Outdated Show resolved Hide resolved
explorer/templatetags/explorer_tags.py Outdated Show resolved Hide resolved
explorer/templates/explorer/query_favorite_button.html Outdated Show resolved Hide resolved
explorer/templates/explorer/query_favorite_button.html Outdated Show resolved Hide resolved
explorer/templatetags/explorer_tags.py Outdated Show resolved Hide resolved
explorer/templatetags/explorer_tags.py Outdated Show resolved Hide resolved
explorer/views/query.py Show resolved Hide resolved
explorer/templates/explorer/base.html Outdated Show resolved Hide resolved
Issue chrisclark#119

Changes from code review
# Conflicts:
#	explorer/tests/test_views.py
@lawson89 lawson89 marked this pull request as ready for review January 20, 2023 10:45
@marksweb
Copy link
Collaborator

@lawson89 Thanks for all your work on this!

@marksweb marksweb merged commit 700db0e into chrisclark:master Jan 21, 2023
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.

None yet

3 participants