Skip to content

Conversation

@shochdoerfer
Copy link
Contributor

As discussed in #575 introduce a setting for the organization to define the default for new packages.

When a new organization is created the default value for the enable_security_scan configuration is true to stick to the previous defaults. The setting can be changed afterward in the respective form like this:
scan

When a new package is created the default value for enable_security_scan flag on the package level is set to the value defined in the organization settings.

As you can see in the screenshot above, changing the setting on the organization level does not change the settings for all packages of that organization. It just will change the default value for newly created packages.

@codecov
Copy link

codecov bot commented May 21, 2022

Codecov Report

Merging #591 (7b8b6a2) into master (8b1d42e) will decrease coverage by 0.02%.
The diff coverage is 96.49%.

@@             Coverage Diff              @@
##             master     #591      +/-   ##
============================================
- Coverage     99.14%   99.11%   -0.03%     
- Complexity     1910     1923      +13     
============================================
  Files           301      304       +3     
  Lines          6072     6117      +45     
============================================
+ Hits           6020     6063      +43     
- Misses           52       54       +2     
Impacted Files Coverage Δ
src/Query/Api/Model/Organization.php 90.90% <60.00%> (-9.10%) ⬇️
src/Controller/Organization/PackageController.php 100.00% <100.00%> (ø)
src/Controller/OrganizationController.php 100.00% <100.00%> (ø)
src/Entity/Organization.php 100.00% <100.00%> (ø)
.../Form/Type/Organization/EnableSecurityScanType.php 100.00% <100.00%> (ø)
src/Message/Organization/AddPackage.php 100.00% <100.00%> (ø)
...e/Organization/ChangeSecurityScanConfiguration.php 100.00% <100.00%> (ø)
.../MessageHandler/Organization/AddPackageHandler.php 100.00% <100.00%> (ø)
...Handler/Organization/EnableSecurityScanHandler.php 100.00% <100.00%> (ø)
...ry/Api/OrganizationQuery/DbalOrganizationQuery.php 100.00% <100.00%> (ø)
... and 2 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 8b1d42e...7b8b6a2. Read the comment docs.

Copy link
Member

@akondas akondas left a comment

Choose a reason for hiding this comment

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

Looks good

$this->hasAnonymousAccess = $hasAnonymousAccess;
}

public function enableSecurityScan(bool $enableSecurityScan): void
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function enableSecurityScan(bool $enableSecurityScan): void
public function changeSecurityScanAvailability(bool $enabled): void

?

return $this->hasAnonymousAccess;
}

public function isSecurityScanEnabled(): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Could all methods/properties referring security scan be unified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't follow. What do you mean by "unified"? Can you give a better example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mix of is and has

$message->metadata(),
$message->keepLastReleases()
$message->keepLastReleases(),
$message->hasSecurityScanEnabled()
Copy link
Contributor

Choose a reason for hiding this comment

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

You have the organization here in the handler, you do not have to pass this feature through a message object.

$organization = $this->organization->getById(Uuid::fromString($message->organizationId()));
$organization->addPackage(new Package(..., $organization->hasEnabledSecurityScan()));

use Ramsey\Uuid\Uuid;
use Symfony\Component\Messenger\Handler\MessageHandlerInterface;

final class EnableSecurityScanHandler implements MessageHandlerInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
final class EnableSecurityScanHandler implements MessageHandlerInterface
final class ChangeSecurityScanConfigurationHandler implements MessageHandlerInterface

@jarzebowsky
Copy link

Any update on this?

@shochdoerfer
Copy link
Contributor Author

good question ;) Sadly I completely lost track of this issue due to a lot of other things I had to focus on.

Let me know which of the findings need to be fixed before the PR can be merged and I'll see that I can take care soonish.

@marmichalski marmichalski self-assigned this Dec 19, 2023
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.

4 participants