Add cities and countries to DFC affiliate sales data endpoint

[chahmedejaz]

What? Why?

• Closes Add cities and countries to DFC affiliate sales data endpoint #12890
• This PR adds country and city data in the affiliate sales data endpoint for supplier and distributor

What should we test?

• As mentioned in the issue

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

[rioug]

Nice one 👍

[dacook]

Looks good, nice and straightforward. Sorry for the delay in review.

[RachL]

@chahmedejaz just checking is this URL correct? https://staging.coopcircuits.fr/api/dfc/affiliate_sales_data?endDate=2024-12-31&startDate=2024-01-01

[chahmedejaz]

@chahmedejaz just checking is this URL correct? https://staging.coopcircuits.fr/api/dfc/affiliate_sales_data?endDate=2024-12-31&startDate=2024-01-01

Yes, @RachL - I believe it should be this. Plus please make sure you are login to the staging server and DFC is connected in the connected apps. Thanks.

[mkllnk]

Great. I updated the docs as well which demonstrate example data.

[RachL]

@chahmedejaz Yes Ioggedin (otherwise I land on a blanck page).

I'm landing on the api but when I try to expand the data, there is no info:

I'm sensing the answer is obvious and I'm not looking at the right place 😅

[chahmedejaz]

@RachL - To validate this, please follow these steps:

• Enable the following feature flags:
  • affiliate_sales_data
  • connected_apps
• Login as an enterprise owner/manager and make sure that this Enterprise has distributed some orders within the start and end date range
• Please "Allow Data Sharing" for the above enterprise by navigating to the Connected Apps panel on the enterprise edit page

After following the above steps please try the API endpoint again. I think the enterprise you are testing with is not allowing data sharing 😅
Please let me know if you face any issues, thanks

[RachL]

Thanks @chahmedejaz with the fact that the staging data has changed since my first tests I completely forgot my setup was incomplete, sorry for the waste of time 🤦‍♀️

However I now get an error 500 when reaching the URL. I'm using enterprise ID 5714 on staging FR, let me know if you want super admin access and I can upgrade your account.

[chahmedejaz]

No worries, @RachL
Please create my account in staging fr and I'll check it.
I'd need admin and enterprise owner accounts. Sharing my email with you in person, please let me know if you need anything. Thanks

[filipefurtad0]

Thanks @chahmedejaz with the fact that the staging data has changed since my first tests I completely forgot my setup was incomplete, sorry for the waste of time 🤦‍♀️

Apologies... This was me trying to make another PR work. I've restored the test data to a previous point. I guess this interefered with your testing @RachL , sorry for that :-/

I guess restoring the DB needs to be used with care, to prevent such situations ⚠️

[filipefurtad0]

I've just noticed the staging-FR label was on this PR. I thought it was not, because of this notice:

The "removed" comes last, so I assumed the server was available. I hope I did not interfere (yet again!) with your testing @RachL . I've re-added the label.

[RachL]

@filipefurtad0 no don't worry I've left it for Ahmed to look at it.

[chahmedejaz]

@RachL - The issue is that your range has a lot of data. This takes time and our server timeout before returning the response
Please shorten the range, like try the data for 1 or 2 months. I tried with
https://staging.coopcircuits.fr/api/dfc/affiliate_sales_data?startDate=2024-10-01&endDate=2024-12-31
And it works as expected. Thanks

[RachL]

I knew the problem was simple!! thanks @chahmedejaz merging :)

[RachL]

@chahmedejaz there is one failling test, do you confirm it's ok to merge?

[chahmedejaz]

That's strange @RachL , this failing spec is passed in the master as well as in this branch on local.
Moreover it's not even related to the changes.
This was passed in the PR before as well. 😅

Will look into it a bit more in a while if I can find something.

Just a tip for the future, unless we have conflict or latest-master-dependant failing spec, we don't need to rebase or force push the branch. 🙂

[RachL]

Just a tip for the future, unless we have conflict or latest-master-dependant failing spec, we don't need to rebase or force push the branch.

thanks! I could be wrong, but I think I've done the rebase otherwise I wouldn't have the merge button (the message said the branch was not up-to-date with the base branch anymore or something like that).

[sigmundpetersen]

Have re-run the spec quite a few times. The failure seems consistent or at least very flaky.
Since the failure is unrelated to the PR we could choose to merge and monitor if the failure continues in master.

If it fails with the same frequency in master we would have to look into it immediately though.

[chahmedejaz]

Just a tip for the future, unless we have conflict or latest-master-dependant failing spec, we don't need to rebase or force push the branch.

thanks! I could be wrong, but I think I've done the rebase otherwise I wouldn't have the merge button (the message said the branch was not up-to-date with the base branch anymore or something like that).

Ah, you did the rebase from GitHub. Sorry for my misunderstanding, I thought you did it by terminal for something 😅

[chahmedejaz]

Have re-run the spec quite a few times. The failure seems consistent or at least very flaky.
Since the failure is unrelated to the PR we could choose to merge and monitor if the failure continues in master.

If it fails with the same frequency in master we would have to look into it immediately though.

Thanks, Sigmund. Let me do some more digging on it and get back to you before we merge

[chahmedejaz]

I tried validating this by pushing only my changes before rebasing, build got green.
When I rebased and pushed, it's still failing the same spec.

I looked into it more closely, the failing spec, I think I've found a way to fix this. But still it's strange that the changes are not related to that spec and it's still changing its behavior 😅

Anyhow, I'll fix this spec here and push the fix as soon as possible.

PS: the failing spec always passes in local setup in both cases, before and after rebase

[chahmedejaz]

Even though these specs are unrelated to this PR's changes, rebasing with the latest master does cause these spec failures.

The reason was that somehow, product names were different in the report and the actual order line item variant product.
In the line_item factory, these products are created with random names. So, in this PR, I've updated those product names to have constant values.

[chahmedejaz]

@sigmundpetersen @RachL - The specs are fixed now, there was some specs data issue. Data has been fixed and specs got fixed now.

The PR functionality has not changed, nor was it related to the previous failure. Please see, if we are good to merge this as PR has been passed the validation.
Otherwise, we could move it to the next step for review and re-validation. Thanks.

[sigmundpetersen]

Let's get a short review from a dev on the last commit, after that it's good to merge I think 👍

[RachL]

@chahmedejaz before this get reviewed again I apologies but I need to ask you to remove the city. I've received an email from the researchers and apparently there is a legal issue about the city name 👀 Sorry about this, I'm really surprise to hear about this so late in the project. Please do put all your hours in clokify / CoopCircuits will pay for that extra work anyway.

[chahmedejaz]

Sure @RachL - No worries.
I'm moving it back to In-Progress and will push the updates shortly. Thanks.

[chahmedejaz]

City has been removed and the changes are ready for review. Thanks