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

Adding the engineer view #266

Merged
merged 10 commits into from
Mar 25, 2024
Merged

Adding the engineer view #266

merged 10 commits into from
Mar 25, 2024

Conversation

ptrnull
Copy link
Contributor

@ptrnull ptrnull commented Mar 8, 2024

Adding a new view that shows info per engineer:

  • Adding a new function to get the user stats: generate_user_stats
  • Adding a new parameter 'engineer' on generate_histogram_stats to get the histogram data filtered by user
  • Adding a new parameter 'engineer' on get_new_comments
  • Adding a new ui template to show the engineer data: engineer.html
  • Modifying macros.html to get the users with a link to their views
  • Modifying ui.py to have a new route defined

Copy link
Collaborator

@adhil0 adhil0 left a comment

Choose a reason for hiding this comment

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

The changes look good in my dev environment, and I'll think this feature will be very useful for all of the engineers.

Our linting check is throwing a couple of errors, though:

  1. There are a couple of lines which exceed 88 characters. This can be fixed by running black or ruff format. (Unless you you disagree with the 88 char limit, in which case we can have a discussion around that)
  2. generate_user_stats() is quite similar to generate_stats(), could we combine the two into a single, generalized function? Just to reduce duplication.

NOTE: The duplicated code detector is being too aggressive (it's flagging this innocent code as a duplication). #268

NOTE: This PR has made me realize that there is quite a bit of duplication across all of our jinja templates, I've opened an issue to address this #267

dashboard/src/t5gweb/ui.py Outdated Show resolved Hide resolved
dashboard/src/t5gweb/templates/ui/engineer.html Outdated Show resolved Hide resolved
@ptrnull ptrnull requested review from adhil0 and dcritch March 13, 2024 14:23
Copy link
Collaborator

@adhil0 adhil0 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the linting issues and combining some functions, it's looking really good now. I found one problem in generate_stats() which is causing the stats table to report incorrect numbers - I've added more details about this problem below

dashboard/src/t5gweb/libtelco5g.py Outdated Show resolved Hide resolved
@ptrnull ptrnull requested a review from adhil0 March 22, 2024 08:00
@dcritch
Copy link
Contributor

dcritch commented Mar 22, 2024

the 1 lint issue it is complaining about isn't a big deal in my opinion. however if you wanted to make a new function in utils.py so:

pie_stats = make_pie_dict(stats)

and in utils.py:

def make_pie_dict(stats):
    return {
        "by_severity": (
            list(stats["by_severity"].keys()),
            list(stats["by_severity"].values()),
        ),
        "by_status": (
            list(stats["by_status"].keys()),
            list(stats["by_status"].values()),
        ),
    }

that'd be swell. then update both get_engineer and get_account to use it.

@adhil0
Copy link
Collaborator

adhil0 commented Mar 22, 2024

the 1 lint issue it is complaining about isn't a big deal in my opinion. however if you wanted to make a new function in utils.py so:

pie_stats = make_pie_dict(stats)

and in utils.py:

def make_pie_dict(stats):
    return {
        "by_severity": (
            list(stats["by_severity"].keys()),
            list(stats["by_severity"].values()),
        ),
        "by_status": (
            list(stats["by_status"].keys()),
            list(stats["by_status"].values()),
        ),
    }

that'd be swell. then update both get_engineer and get_account to use it.

+1 - on second thought, it's probably better to put this in a function since we're using the code in multiple places

Copy link
Contributor

@dcritch dcritch left a comment

Choose a reason for hiding this comment

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

one minor nit, then you're good to go. thanks!

dashboard/src/t5gweb/libtelco5g.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dcritch dcritch left a comment

Choose a reason for hiding this comment

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

thanks for the contribution and for dealing with all the back and forth :) lgtm!

Copy link
Collaborator

@adhil0 adhil0 left a comment

Choose a reason for hiding this comment

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

Thanks, looking really good now

@adhil0 adhil0 merged commit dd872e1 into RHsyseng:main Mar 25, 2024
2 checks passed
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