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

Fixes #181 (WIP) #382

Merged
merged 5 commits into from Feb 7, 2022
Merged

Fixes #181 (WIP) #382

merged 5 commits into from Feb 7, 2022

Conversation

paulosgf
Copy link
Contributor

  • Added PR Github Author column to the dashboard.

What's Changed

A description of the issue/feature; reference the issue number (if one exists). The Pull Request should not include fixes for issues other than the main issue/feature request.

Technical Description

Any specific technical detail that may provide additional context to aid code review.

Before opening a Pull Request you should read and agreed to the Contributor Code of Conduct (see CONTRIBUTING.md)

@paulosgf paulosgf marked this pull request as draft January 22, 2022 20:53
@paulosgf paulosgf marked this pull request as ready for review January 22, 2022 20:54
@paulosgf
Copy link
Contributor Author

Hi @apoclyps!

I'm making a new push with some changes.
To implement this enhancement with GitLab, I modified the following files:

On code_review_requests tuple of GitlabPullRequestController class, i added the entry user=pull_request.author['username']

reviews/controller.py
class GitlabPullRequestController():

code_review_requests.append(
                PullRequest(
                  user=pull_request.author['username'],

This entry is referenced by both classes GitlabPullRequestController and GithubPullRequestController, and is called in a row on method render_pull_request_table()

reviews/layout/helpers.py
  def render_pull_request_table(
            row = [
            f"[white]{pr.number} ",
            pr.render_title(),
            f"[white]{pr.user} ",
            pr.render_labels(label_colour_map),
        ]

To get pull requests from GitlabPullRequestController class it's needed to exchange list() by theget()method and pass the project ID as function's argument.

reviews/source_control/client.py
class GitlabAPI:

 def get_pull_requests(self, project_id: str, namespace: str) -> List[GitlabMergeRequest]:
  return repository.mergerequests.get(id=project_id, state="opened", order_by="created_at", sort="asc")

But, contrary to what happens with the code_review_requests() method of GithubPullRequestController class, on GitlabPullRequestController class it doesn't find any pull request and returns this error:

File "/home/paulo/PycharmProjects/reviews/venv/lib/python3.8/site-packages/gitlab/client.py", line 727, in http_request
    raise gitlab.exceptions.GitlabHttpError(
  gitlab.exceptions.GitlabHttpError: 404: 404 Not found

I have debugged a while and find that on ~/.local/lib/python3.8/site-packages/gitlab/mixins:111 the query_data = {} variable is empty when is made GET request to the Gitlab server:

def http_get(query_data: Optional[Dict[str, Any]] = None,
"""Make a GET request to the Gitlab server
Args:
               path: Path or full URL to query ('/projects' or 'http://whatever/v4/api/projecs')                          
               query_data: Data to send as query parameters

Then, thereafter, I didn't make any progress anymore. Could you give some help?

@apoclyps
Copy link
Owner

@paulosgf sure, I'll try and set some time aside after work this evening to review this pull request and provide assistance on where to go next and how to proceed 👍

paulosgf and others added 5 commits February 6, 2022 11:54
@codecov
Copy link

codecov bot commented Feb 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@1c95f30). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #382   +/-   ##
=======================================
  Coverage        ?   18.61%           
=======================================
  Files           ?       21           
  Lines           ?      623           
  Branches        ?        0           
=======================================
  Hits            ?      116           
  Misses          ?      507           
  Partials        ?        0           
Flag Coverage Δ
unittests 18.61% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c95f30...280630f. Read the comment docs.

@apoclyps
Copy link
Owner

apoclyps commented Feb 6, 2022

@paulosgf thank you for the detailed comment; I've contracted covid-19 last week so it's taken me slightly longer to return to looking into/reviewing this pull request.

After spending some time looking into the issue with Gitlab, it appears to be a change related to a MergeRequest and a ProjectMergeRequest; you can see the changes I made in the third pull request to resolve this change.

As for the configuration for show/hide author - I made this an ENV var to avoid passing it through multiple functions/classes from the CLI. Overall reviews is in need of an extensive refactor - I'm currently looking into building a new self-hosted web-based version to replace the CLI.

  • [refactors user to author] in (5e4f8e1)
  • [adds configuration to show/hide author] in (026864b)
  • [updates gitlab to handle changes to their API] in 280630f)

Thank you for your contribution; I hope the above commits help with understanding the few remaining pieces needed to get this across the line 👍

@apoclyps
Copy link
Owner

apoclyps commented Feb 6, 2022

Screenshots

python -m reviews dashboard --no-reload --provider=github

image

export REVIEWS_AUTHOR=false
python -m reviews dashboard --no-reload --provider=github

image

@apoclyps apoclyps merged commit 8da0734 into apoclyps:main Feb 7, 2022
@paulosgf
Copy link
Contributor Author

paulosgf commented Feb 7, 2022

@apoclyps I'm so glad that you're good now! It must be a terrible experience in all aspects... but unfortunately, all of us are subject to this.
I would like to thank you for your help with this. It was a great experience and I learned a lot from it!
Count with me, within my possibilities, for help on refactoring to a web-based version. Maybe Django? It's my next step in learning.
Embraces!

@apoclyps
Copy link
Owner

I've currently been considering FastAPI and celery as it seems like a good opportunity to try out some new tech. I'm certain that Django is more than capable as I've used it for several projects in the past however I'm curious to see how minimalist I can build it - in order to help others easily onboard and extend the solution.

@paulosgf paulosgf deleted the authorpullreq branch February 10, 2022 19:09
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

2 participants