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

3.17 - Activation/Deactivation with the filters #6878

Closed
piotrbak opened this issue Aug 20, 2024 · 9 comments · Fixed by #6928
Closed

3.17 - Activation/Deactivation with the filters #6878

piotrbak opened this issue Aug 20, 2024 · 9 comments · Fixed by #6928
Assignees
Labels
type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

piotrbak commented Aug 20, 2024

Context
We need to have filters to enable/disable both OCI and LRC features

Acceptance Criteria

@piotrbak piotrbak added the type: enhancement Improvements that slightly enhance existing functionality and are fast to implement label Aug 20, 2024
@piotrbak piotrbak added this to the 3.17 milestone Aug 20, 2024
@Miraeld
Copy link
Contributor

Miraeld commented Aug 26, 2024

Hello hello,

It seems to me the AC #1 is already covered.

For AC #2, @piotrbak could you confirm something: If one of these features: LRC OR OCI is disabled, we still display the clearing button for both features?

Thank you

@Miraeld
Copy link
Contributor

Miraeld commented Aug 26, 2024

Otherwise, for AC 3,

Scope a solution

In

public function display_dashboard_button() {
, we would need to add the following:

		$all_not_allowed = true;

		foreach ($this->factories as $factory) {
			if ($factory->get_context()->is_allowed()) {
				$all_not_allowed = false;
				break;
			}
		}

		if ($all_not_allowed) {
			return;
		}

In this case, if every factories return false to is_allowed() we will not display the option to clear the data.

Also, for AC 2, it will shows the option if at least one feature is enabled.

@Khadreal
Copy link
Contributor

Khadreal commented Aug 26, 2024

The factories are loaded if the context is true, so we could just check if it's empty at this point. Also, the lrc_factory context check is missing there.

if ( $atf_factory->get_context()->is_allowed() ) {

[Edit]
The dashboard button trait class already check for this

if ( ! $context ) {
return;
}
. What we need to do will be to load the lrc_factory in the serviceprovider class

@piotrbak
Copy link
Contributor Author

@Miraeld If one of these features: LRC OR OCI is disabled, we still display the clearing button for both features?

Yes, that's correct

@Khadreal
Copy link
Contributor

@Miraeld I can see factories is being added, what we need to do will be to test this scenarios esp AC 3 to confirm everything is fine

@Miraeld
Copy link
Contributor

Miraeld commented Aug 27, 2024

Yes,

For AC 1 & 2, its already working like we want.
However for 3 we need to implement what I've recommended earlier.

Also, for all three scenarios, we need to implement tests to make sure it's working as expected.

@jeawhanlee
Copy link
Contributor

@Miraeld @Khadreal I don't think we have issues with the clear button displays whether on the dashboard or the admin bar menu.
What I think we need to check is if we clear the data as expected for those 3 ACs

@Khadreal
Copy link
Contributor

@jeawhanlee spot on. I think we're good on that. The data cleared as expected. Moving this to QA

@Khadreal
Copy link
Contributor

Video recording showing the filter activation and deactivation
https://www.loom.com/share/f02029def0e84c3d94a331de489f5b8b?sid=509959fd-2729-4bd7-b7af-4174fea70afb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants