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

Remove legacy report date parameters. #6395

Closed
techanvil opened this issue Jan 12, 2023 · 6 comments
Closed

Remove legacy report date parameters. #6395

techanvil opened this issue Jan 12, 2023 · 6 comments
Labels
Exp: SP P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 12, 2023

Feature Description

Following on from https://github.com/google/site-kit-wp/pull/6379/files#r1067422019, we should remove the legacy dateRange, compareDateRanges, and multiDateRange report options from the codebase.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The codebase should be audited for occurrences of the report options dateRange, compareDateRanges, and multiDateRange.
  • These should be removed or replaced as appropriate.

Implementation Brief

  • Using your editor, search for explicit matches of dateRange, compareDateRanges, and multiDateRange in PHP and JS files.
  • Jump from result to result and remove the parts where the above entities are declared/updated and their associated logic.
  • NOTE! Make sure that we're only removing instances of dateRange, compareDateRanges, and multiDateRange that relate to the options for a report eg:
    * @param {string} options.dateRange Required, alternatively to startDate and endDate. A date range string. Default 'last-28-days'.

Test Coverage

  • Update any failing tests.

QA Brief

  • There is not much to QA here the aforementioned properties are not used by us, thus there is no user facing changes but we can do QA:eng here by checking whether we still have remaining usages of those parameters in the codebase.

Changelog entry

  • N/A.
@techanvil techanvil added P2 Low priority Type: Enhancement Improvement of an existing feature labels Jan 12, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jan 13, 2023
@tofumatt
Copy link
Collaborator

ACs here are good, moving to IB 👍🏻

@sashadoes sashadoes assigned sashadoes and unassigned sashadoes Jan 17, 2023
@techanvil
Copy link
Collaborator Author

techanvil commented Jan 20, 2023

Hey @sashadoes, the IB should also cover the fact that we have some legacy usage of these parameters in JS files as well which need to be assessed and removed.

@techanvil techanvil assigned techanvil and sashadoes and unassigned techanvil Jan 20, 2023
@sashadoes sashadoes assigned techanvil and unassigned sashadoes Jan 24, 2023
@techanvil
Copy link
Collaborator Author

Thanks for updating the IB @sashadoes. I think we still need to be a bit clearer about dateRange in particular (although the wording can apply to all of the terms). Searching for dateRange in the JS will return quite a lot of hits which are valid and we want to keep.

We need to ensure that we're only removing instances of dateRange that relate to the options for a report. This should be clear from reading the AC, but I think the IB should also be a bit clearer in this regard.

Also, is the estimate bumped up by one, i.e. should the Exp: SP label be added?

@techanvil techanvil assigned sashadoes and unassigned techanvil Jan 24, 2023
@sashadoes sashadoes assigned techanvil and unassigned sashadoes Jan 27, 2023
@sashadoes
Copy link
Collaborator

sashadoes commented Jan 27, 2023

HI @techanvil , Thanks for the review and feedback.
I made an update.
I put 11SP with Exp: SP as we could have some failing tests as well.

@techanvil
Copy link
Collaborator Author

Thanks @sashadoes (btw, watch out for that autoselect/typo choosing @tofumatt instead of @techanvil :))

I've changed the Test Coverage point to read "Update any failing tests.", apart from that, LGTM.

IB ✅

@eugene-manuilov eugene-manuilov removed their assignment Aug 17, 2023
@techanvil
Copy link
Collaborator Author

techanvil commented Aug 17, 2023

QA ✅

As @eugene-manuilov mentioned, there isn't much to QA here, although I have nonetheless given it a smoke test to exercise the Analytics, AdSense and Search Console reports.

I've also confirmed that there are no remaining references to compareDateRanges or multiDateRange in the codebase, and the only remaining references to dateRange are for unrelated matters.

Therefore, I'm moving this directly to Approval.

@techanvil techanvil removed their assignment Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants