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 Root Route for Datagov Harvest Admin #141

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

btylerburton
Copy link
Contributor

@btylerburton btylerburton commented Feb 11, 2025

Pull Request

Related to:

About

Also:

  • Improves pagination module
  • Adds pagination to harvest source summary page
  • Drops pagination default to 10 entries

PR TASKS

  • Code well documented
  • Tests written, run and passed
  • Files linted

@btylerburton btylerburton changed the title Redirect Admin Root Redirect Admin Root for Harvester Flask Admin Feb 11, 2025
@btylerburton btylerburton changed the title Redirect Admin Root for Harvester Flask Admin Redirect Root Route for Harvester Flask Admin Feb 11, 2025
@btylerburton btylerburton changed the title Redirect Root Route for Harvester Flask Admin Redirect Root Route for Datagov Harvest Admin Feb 11, 2025
@btylerburton btylerburton requested a review from a team February 11, 2025 22:47
neilmb
neilmb previously approved these changes Feb 12, 2025
Copy link

@neilmb neilmb left a comment

Choose a reason for hiding this comment

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

LGTM, my comments are just bossy quibbles, nice work.

app/routes.py Outdated
# TODO: wire in paginated jobs htmx refresh ui & route
jobs = db.pget_harvest_jobs(
paginate=False, facets=f"harvest_source_id = '{source.id}'"
)
next_job = "N/A"
if len(jobs):
Copy link

Choose a reason for hiding this comment

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

comment (non-blocking): I think that the most idiomatic version of this might be if jobs:, or maybe if len(jobs) > 0, but it's also fine as-is.

app/routes.py Outdated
# TODO: wire in paginated jobs htmx refresh ui & route
jobs = db.pget_harvest_jobs(
paginate=False, facets=f"harvest_source_id = '{source.id}'"
)
next_job = "N/A"
if len(jobs):
last_job = jobs[len(jobs) - 1]
Copy link

Choose a reason for hiding this comment

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

comment (non-blocking): More idioms, negative indexes count from the end so last_job = jobs[-1] works the same as what you have here.

app/routes.py Outdated Show resolved Hide resolved
cmhedrickREI
cmhedrickREI previously approved these changes Feb 12, 2025
Copy link
Member

@cmhedrickREI cmhedrickREI left a comment

Choose a reason for hiding this comment

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

I think this looks good! Thanks for fixing the broken test cases!

redirects to all orgs page on load;
udpates harvest source page with feedback changes;
updates fixtures to have better jobs data;
fixes broken tests;
lint
cleans up metrics route code;
fixes issue with pagination numbers in pagination module;
makes pagination per_page count a kwarg;
removes sort functionality from tables as it breaks with htmx updates and is not super useful
calculates count using new count decorator
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.

3 participants