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

GH-2289: Add sub status to CMP fetch URL #694

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

@leuryr
Copy link
Contributor

@leuryr leuryr commented Feb 26, 2021

Adds the ss param to the CMP fetch URL. Feeling a bit conflicted about having the logic for this inside the _buildUrl method, though it is technically part of building the URL. Open to moving it into a separate function.


  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • Did you lint your code prior to submission?
@leuryr leuryr requested a review from christophertino as a code owner Feb 26, 2021
@wlycdgr wlycdgr self-requested a review Mar 2, 2021
@wlycdgr
Copy link
Member

@wlycdgr wlycdgr commented Mar 2, 2021

Looks like it works as expected, but I share your mixed feelings about having the logic be inside the _buildURL method. I suggest factoring it out to a static method of either the CMP or the Metrics class. While you're at it, could you convert CMP#_buildURL to use the _buildQueryPair method you made in Metrics? It can be (and should be, since it can be) converted to a static method, I think.

Probably most importantly, we should see if we can rename the key from ss to something else, to avoid the confusion of a key that means one thing in the context of a call to the CMP server and something else entirely in the context of a call to the metrics server. Let's work with JJ to see how we can do this - technically, it should be an easy change on the CMP server side.

Copy link
Member

@wlycdgr wlycdgr left a comment

See comment (but disregard the part about the parameter naming)

@wlycdgr wlycdgr changed the title GH-2289: Add sub status to CMP fetch URL (REVIEWED; COMMENTS TO BE ADDRESSED) GH-2289: Add sub status to CMP fetch URL Mar 3, 2021
@leuryr leuryr requested a review from ghostery/extension as a code owner Mar 8, 2021
@leuryr
Copy link
Contributor Author

@leuryr leuryr commented Mar 8, 2021

As we discussed, I moved the query string building to a util function that we can use for both the CMP & Metrics classes (with a small addition for when we need a ? at the beginning of the returned string instead of &), and I factored getting the sub status out from CMP_buildURL into its own static method.

@wlycdgr wlycdgr changed the title (REVIEWED; COMMENTS TO BE ADDRESSED) GH-2289: Add sub status to CMP fetch URL GH-2289: Add sub status to CMP fetch URL Mar 8, 2021
@wlycdgr
wlycdgr approved these changes Mar 9, 2021
Copy link
Member

@wlycdgr wlycdgr left a comment

LGTM

@wlycdgr wlycdgr modified the milestones: 8.5.7, 8.5.6 Mar 9, 2021
@wlycdgr wlycdgr removed this from the 8.5.6 milestone Mar 10, 2021
@wlycdgr wlycdgr added this to the 8.5.7 milestone Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants