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 #3140 - Publish private issue in public after moderation #3167

Merged
merged 22 commits into from Feb 5, 2020

Conversation

@karlcow
Copy link
Contributor

karlcow commented Jan 22, 2020

This PR fixes issue #3140

closes #3140
closes #3141

Proposed PR background

it refactors the code and makes it easier to read (hopefully).
It makes it possible for issue which have been accepted to be transferred to the public repo.

This basically push the action processing on issues out of the route.
It adds a generic function that can be further refined and handles multiple actions.

Some goals:
* easier to read
* easier to test
* more flexibility for growth
@karlcow

This comment has been minimized.

Copy link
Contributor Author

karlcow commented Jan 22, 2020

Created a draft pull request so people can see the evolution.

karlcow added 5 commits Jan 22, 2020
Still in the process of making the code more readable.

repo_scope will provide a way to know if we are dealing with a public or a private repo.
It makes it easier to scope the actions to certain consequences and it makes it more readable and testable.
I had forgotten that I didn't need to actually put the signature as I am already computing it. That makes things a lot simpler when changing the body of the fixture.
@karlcow karlcow changed the title Fixes #3140 - Adds a process_issue_action for better flexibility Fixes #3140 - Publish private issue in public after moderation Jan 22, 2020
karlcow added 6 commits Jan 23, 2020
That makes it more obvious. `new_opened_issue` was a bit generic.
There is a couple of things to unpack here.

* Simplifies the json fixture so we focus on what we need.
* Adds a test which is handling **only** the HTTP workflow of the webhook
   note they start to repeat a bit, and we probably need to functionalize this a bit at a point. We will see that at the end.
* Moves logging where it should have been in previous commit. My bad.
* Modifies the dictionary we use with the info we need to add a milestoned_with keyword
* Test that the payload has the right criteria before processing it.

This doesn't contain the actual posting to the public repo **yet** Because there will be probably related to #3141 too.
This will make it a bit clearer in the code.
Instead of manipulating two types of data, we focus only on one structured dictionary
Basically we have a structured dictionary which contains all the info we need.
This will be sent to the repo for updating the public issue.

There's still a bit of data massaging to make it proper in `private_issue_moderation`

(Probably once this is all done, we would need to rebase all of this or create a new branch with a proper set of commits.)
@karlcow

This comment has been minimized.

Copy link
Contributor Author

karlcow commented Jan 24, 2020

Status: not finished yet.

But on a good path. I needed to refactor certain things.
To understand what I'm doing the detailed commits are probably interesting to read.

Basically read git log.

karlcow added 3 commits Jan 24, 2020
This creates a separate function in charge of preparing the payload.
- It removes the action-needsmoderation label
- It grabs the required body
- It grabs the title of the issue

This will make it easier to test if/when we change
what we want to send as a payload.
The tests call indirectly extract_priority_label
which triggers a different result if the db is populated or not.
So instead of having a variation, we force the return value
of the priority for this specific case.
@karlcow

This comment has been minimized.

Copy link
Contributor Author

karlcow commented Feb 3, 2020

Opened a new issue related to the test environment stability #3173

karlcow added 4 commits Feb 3, 2020
We need the number to patch the issue at the right place.
We make sure that we send the HTTP PATCH with the right arguments. So basically that we are sending the right information at the right place.

A question though: do we want to send the public_url in the body?
This double checks that the processing for issue moderation is
coming from the right scope. Often probably a wrong repo.
That would prevent us from a copy pasta error.
This just put in place references to the code.
The rest of this could be addressed in this PR
or in a specific PR with commits in #3141
@karlcow

This comment has been minimized.

Copy link
Contributor Author

karlcow commented Feb 3, 2020

@miketaylr Ok good news. This will be finished by tomorrow evening (Tuesday evening JST)

karlcow added 2 commits Feb 4, 2020
continuation of #3140 for handling issue moderation.
The moderation template now handles two keywords ongoing
and rejected. This will help to adjust the code sent
to patch the issue.
This should be the final patch for the new anonymous workflow.

    When the issue has been moderated as rejected,
    we need to change a couple of things in the public space

    - change Title
    - change body
    - close the issue
    - remove the action-needsmoderation label
    - change the milestone to invalid

This is the companion for #3140
Once this is merged on staging, we can activate #3155
on the private test repo, then on the public repo.
@karlcow karlcow marked this pull request as ready for review Feb 4, 2020
@karlcow

This comment has been minimized.

Copy link
Contributor Author

karlcow commented Feb 4, 2020

@miketaylr ok, we are ready and let's hope it doesn't break the world.

@miketaylr miketaylr requested a review from ksy36 Feb 4, 2020
Copy link
Member

miketaylr left a comment

Nice work! I tried to take my time with this (admittendly, skimming over tests).

Just a few questions / nits that can be ignored or taken care of now.

webcompat/webhooks/helpers.py Show resolved Hide resolved
webcompat/webhooks/helpers.py Show resolved Hide resolved
webcompat/webhooks/helpers.py Outdated Show resolved Hide resolved
webcompat/webhooks/helpers.py Show resolved Hide resolved
webcompat/webhooks/helpers.py Outdated Show resolved Hide resolved
webcompat/webhooks/helpers.py Show resolved Hide resolved
webcompat/webhooks/helpers.py Outdated Show resolved Hide resolved
webcompat/webhooks/helpers.py Outdated Show resolved Hide resolved
webcompat/webhooks/helpers.py Outdated Show resolved Hide resolved
webcompat/webhooks/helpers.py Show resolved Hide resolved
@ksy36
ksy36 approved these changes Feb 4, 2020
Part of #3141 This also addresses comments on the PR review
@karlcow

This comment has been minimized.

Copy link
Contributor Author

karlcow commented Feb 5, 2020

I addressed the comments of @miketaylr in
8ec394b

@karlcow

This comment has been minimized.

Copy link
Contributor Author

karlcow commented Feb 5, 2020

Let's see in circleci is happy about the changes.

@karlcow karlcow merged commit d8797bd into webcompat:master Feb 5, 2020
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@karlcow karlcow deleted the karlcow:3140/1 branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.