swissspidy commented 5 years ago

Description This adds new methods to the Project class to set and retrieve webhook secrets.

Since the secrets are needed in the permission callbacks the webhook handlers are updated accordingly to locate the projects earlier in the process.

Fixes #80.

How has this been tested?

Unit tests still work 🙂

Types of changes Enhanced Project class, added new protected methods to the webhook handlers with a new traduttore.webhook_secret filter.

Checklist:

[x] My code is tested.
[x] My code follows the WordPress code style.
[x] My code has proper inline documentation.

codecov-io commented 5 years ago

Codecov Report

Merging #91 into master will increase coverage by 0.83%. The diff coverage is 94.59%.

@@            Coverage Diff            @@
##             master   #91      +/-   ##
=========================================
+ Coverage     74.16%   75%   +0.83%     
- Complexity      322   334      +12     
=========================================
  Files            23    23              
  Lines           774   804      +30     
=========================================
+ Hits            574   603      +29     
- Misses          200   201       +1

Impacted Files	Coverage Δ	Complexity Δ
inc/WebhookHandler/GitHub.php	`100% <100%> (+2.85%)`	`12 <0> (-1)`	:arrow_down:
inc/WebhookHandler/Bitbucket.php	`91.89% <100%> (+0.98%)`	`12 <0> (ø)`	:arrow_down:
inc/WebhookHandler/GitLab.php	`96.96% <100%> (+0.41%)`	`11 <0> (ø)`	:arrow_down:
inc/WebhookHandler/Base.php	`100% <100%> (ø)`	`10 <9> (+9)`	:arrow_up:
inc/Project.php	`96.22% <60%> (-3.78%)`	`40 <4> (+4)`

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 1013216...e829c04. Read the comment docs.

swissspidy commented 5 years ago

You can't put non-public methods in an interface, but we could certainly add get_secret() to Base. I didn't because all the methods are slightly different because of the constants. But I guess we could use a switch statement or if in a Base::get_secret() method instead.

grappler commented 5 years ago

@swissspidy What about writing the secret code like this?

    $secret = $project ? $project->get_repository_webhook_secret() : null;
    if ( ! $secret ) {
        switch ( get_class( $this ) ) {
            case Bitbucket::class:
                if ( defined( 'TRADUTTORE_BITBUCKET_SYNC_SECRET' ) ) {
                    $secret = TRADUTTORE_BITBUCKET_SYNC_SECRET;
                }
                break;
            case GitHub::class:
                if ( defined( 'TRADUTTORE_GITHUB_SYNC_SECRET' ) ) {
                    $secret = TRADUTTORE_GITHUB_SYNC_SECRET;
                }
                break;
            case GitLab::class:
                if ( defined( 'TRADUTTORE_GITLAB_SYNC_SECRET' ) ) {
                    $secret = TRADUTTORE_GITLAB_SYNC_SECRET;
                }
                break;
        }
    }

swissspidy commented 5 years ago

I have no strong preference. I would probably go with the current solution because it's already there, has similar readability, and all further discussion would only lead to bikeshedding :)

wearerequired / traduttore

Support per-project webhook secrets #91

Codecov Report