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

Expand user #3

Conversation

ilyakubanov
Copy link
Collaborator

@ilyakubanov ilyakubanov commented Aug 5, 2024

/**
* Specification:
* - Iterates over `UserCollectionTransfer.users`.
* - Requires `UserTransfer.idUser` for each user in collection to be set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Requires... part usually goes first in the specification.

*
* @return list<int>
*/
protected function getUserIds(UserCollectionTransfer $userCollectionTransfer): array
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
protected function getUserIds(UserCollectionTransfer $userCollectionTransfer): array
protected function extractUserIds(UserCollectionTransfer $userCollectionTransfer): array

*
* @return \Generated\Shared\Transfer\UserCollectionTransfer
*/
public function expandUserCollectionWithQuicksightUser(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function expandUserCollectionWithQuicksightUser(
public function expandUserCollectionWithQuicksightUsers(

* Use of this software requires acceptance of the Evaluation License Agreement. See LICENSE file.
*/

declare(strict_types=1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, remove this line

<property name="role" dataBuilderRule="lexify('??????????')"/>
<property name="arn" dataBuilderRule="lexify('??????????')"/>
<property name="principalId" dataBuilderRule="lexify('??????????')"/>
<property name="uuid" dataBuilderRule="unique()->uuid()"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid generating UUIDs in data builders.

* @method \SprykerEco\Zed\AmazonQuicksight\Business\AmazonQuicksightFacadeInterface getFacade()
* @method \SprykerEco\Zed\AmazonQuicksight\Communication\AmazonQuicksightCommunicationFactory getFactory()
*/
class QuicksightUserUserExpanderPlugin extends AbstractPlugin implements UserExpanderPluginInterface
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can make an exception from our conventions and name this plugin QuicksightUserExpanderPlugin to avoid duplication in the class name?

@ilyakubanov ilyakubanov merged commit a30a781 into feature/cc-34041/dev-quick-sight-mvp Aug 6, 2024
2 checks passed
@ilyakubanov ilyakubanov deleted the feature/cc-34041/cc-34137-extend-user-transfer branch August 6, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants