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

Bugfix for experiment data export storing condition name in condition payload column #2095

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

ppratikcr7
Copy link
Collaborator

@ppratikcr7 ppratikcr7 commented Nov 6, 2024

This resolves #2090

@ppratikcr7 ppratikcr7 marked this pull request as ready for review November 7, 2024 12:13
@ppratikcr7 ppratikcr7 requested a review from zackcl November 7, 2024 12:13
@zackcl
Copy link
Collaborator

zackcl commented Nov 7, 2024

@ppratikcr7 Could you please explain how/where this PR resolves the 2 issues mentioned in #2090?

  1. In new data export, the filed for Payload seems to copy data from Condition, rather than including the actual payload. Expected behavior is that the Payload field will represent the experiment payload.

  2. Also, rows are duplicated (see markExperimentPointTime fractional seconds).

Also, does anyone know a good way to test/validate this? I'm not sure how to properly QA this. I don't think I can send experiment data to my email when running locally (I get the CredentialsProviderError: Could not load credentials from any providers error).

If anyone is confident in approving this PR, please feel free to do so.

@ppratikcr7
Copy link
Collaborator Author

ppratikcr7 commented Nov 7, 2024

@ppratikcr7 Could you please explain how/where this PR resolves the 2 issues mentioned in #2090?

  1. In new data export, the filed for Payload seems to copy data from Condition, rather than including the actual payload. Expected behavior is that the Payload field will represent the experiment payload.
  2. Also, rows are duplicated (see markExperimentPointTime fractional seconds).

Also, does anyone know a good way to test/validate this? I'm not sure how to properly QA this. I don't think I can send experiment data to my email when running locally (I get the CredentialsProviderError: Could not load credentials from any providers error).

If anyone is confident in approving this PR, please feel free to do so.

@zackcl You need to update the payload values in the experiment, they are set as condition names by default.

Also, once you have multiple condition payloads for a condition, you should see duplicate rows with a only the condition payload column as different, which earlier would be seen as same due to it populating the condition name for both. You should get an export as below:

[email protected]_simpleExport2024-11-06T14-04-50.850Z.csv

Copy link
Collaborator

@bcb37 bcb37 left a comment

Choose a reason for hiding this comment

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

One question

acc.push(experiment);
}
experiment.details.push({
// add the design details for the experiment for each experimentQueryResult item
experimentMap.get(item.experimentId)?.details.push({
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this was overwriting the details for existing experiments, can't we fix that by simply closing the if statement on line 221 (old 212), rather than creating the map structure?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise, this looks ok, now that we're setting 'payload' and not 'payloadValue'

Copy link
Collaborator Author

@ppratikcr7 ppratikcr7 Nov 8, 2024

Choose a reason for hiding this comment

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

Yes we had that in the old code, I felt using a Map would be more efficient. You want me to revert this part? Wont make much of a difference though.

@danoswaltCL danoswaltCL merged commit 3d0ef14 into release/6.0 Nov 8, 2024
14 checks passed
@danoswaltCL danoswaltCL deleted the hotfix/csv-payload-data-export-issue-2090 branch November 8, 2024 14:37
danoswaltCL added a commit that referenced this pull request Nov 19, 2024
* intial 6.0.0 version bump

* Decoupling join and paginated query

* Apply filterMode (includeAll) to an imported Feature Flag (#2031)

* (Release) Refine Disabled Slide Toggles and Action Buttons on 'Reader' mode  (#2032)

* Refine Disabled Slide Toggles and Action Buttons on 'Reader' mode (Release)

* Move common mat-mdc-slide-toggle styles to styles.scss

* adding exposures to the feature flag paginated query

* Bugfix/handy prerelease version bump script (#2033)

* add prerelease version bump/removal script

* add prerelease bump script and bump to 1

---------

Co-authored-by: Zack Lee <[email protected]>

* v6-pre-release version update

* (Release) Fix Not-Allowed Cursor Appearing on Enabled Slide Toggles (#2040)

* (Release) Fix the Not-Allowed Cursor Bug on Disabled Slide Toggles

* version bump

---------

Co-authored-by: danoswaltCL <[email protected]>

* add var to turn off migrations for ECS (#2041)

* add var to turn off migrations for ECS

* add var to CF config file

* hardcode IS_ECS to 'true' in CF config

* use branch to build artifacts and deploy automatically to staging (#2056)

uses branch as version.

* fix config string replacement (#2054) (#2063)

* fix config string replacement

* set featureFlagNavToggle to true

* add quicktests for the clientlibs (#2081)

* add quicktests for the clientlibs

* commit correct version

* fix log response and java CL log call

---------

Co-authored-by: Benjamin Blanchard <[email protected]>

* Bugfix for experiment data export storing condition name in condition payload column (#2095)

* hotfix for condition payload data eport issue and adding types

* return NA for no enrolment start and mark date time

* removed disable on context change in experiment edit modal (#2094)

* removed disable on context change in experiment edit modal

* Remove logging checkbox and enableSave variable; Add warning message for Context/Design Type changes in Overview step

---------

Co-authored-by: Zack Lee <[email protected]>
Co-authored-by: Zack Lee <[email protected]>

* Revert Logging Property in the Overview Form (#2103)

* fix up package-locks

* hotfix to handle duplicate export data csv rows for marking multiple decision points by same user

(cherry picked from commit ca84d0a)

---------

Co-authored-by: Vivek Fitkariwala <[email protected]>
Co-authored-by: Zack Lee <[email protected]>
Co-authored-by: pratik <[email protected]>
Co-authored-by: Pratik Prajapati <[email protected]>
Co-authored-by: Ben Blanchard <[email protected]>
Co-authored-by: shpwe <[email protected]>
Co-authored-by: Yagnik Hingrajiya <[email protected]>
Co-authored-by: Zack Lee <[email protected]>
danoswaltCL added a commit that referenced this pull request Dec 4, 2024
* intial 6.0.0 version bump

* Decoupling join and paginated query

* Apply filterMode (includeAll) to an imported Feature Flag (#2031)

* (Release) Refine Disabled Slide Toggles and Action Buttons on 'Reader' mode  (#2032)

* Refine Disabled Slide Toggles and Action Buttons on 'Reader' mode (Release)

* Move common mat-mdc-slide-toggle styles to styles.scss

* adding exposures to the feature flag paginated query

* Bugfix/handy prerelease version bump script (#2033)

* add prerelease version bump/removal script

* add prerelease bump script and bump to 1

---------

Co-authored-by: Zack Lee <[email protected]>

* v6-pre-release version update

* (Release) Fix Not-Allowed Cursor Appearing on Enabled Slide Toggles (#2040)

* (Release) Fix the Not-Allowed Cursor Bug on Disabled Slide Toggles

* version bump

---------

Co-authored-by: danoswaltCL <[email protected]>

* add var to turn off migrations for ECS (#2041)

* add var to turn off migrations for ECS

* add var to CF config file

* hardcode IS_ECS to 'true' in CF config

* use branch to build artifacts and deploy automatically to staging (#2056)

uses branch as version.

* fix config string replacement (#2054) (#2063)

* fix config string replacement

* set featureFlagNavToggle to true

* add quicktests for the clientlibs (#2081)

* add quicktests for the clientlibs

* commit correct version

* fix log response and java CL log call

---------

Co-authored-by: Benjamin Blanchard <[email protected]>

* Bugfix for experiment data export storing condition name in condition payload column (#2095)

* hotfix for condition payload data eport issue and adding types

* return NA for no enrolment start and mark date time

* removed disable on context change in experiment edit modal (#2094)

* removed disable on context change in experiment edit modal

* Remove logging checkbox and enableSave variable; Add warning message for Context/Design Type changes in Overview step

---------

Co-authored-by: Zack Lee <[email protected]>
Co-authored-by: Zack Lee <[email protected]>

* Revert Logging Property in the Overview Form (#2103)

* hotfix to handle duplicate export data csv rows for marking multiple decision points by same user (#2117)

* update to 6.0.5, ready for prod (#2118)

* get export log data from replica (#2125)

* update filter mode when ALL row is removed in include table (#2131)

* match package version to build tags (#2138)

* Update clientlib readme (#2139)

* update typedoc generated readme and other docs

* fix quick tests

* readme typedocs and quicktest brush-ups

* update filelockVersion for two packages

* fix up package-locks?

* remove nounused var from tsconfig for build?

---------

Co-authored-by: Vivek Fitkariwala <[email protected]>
Co-authored-by: Zack Lee <[email protected]>
Co-authored-by: pratik <[email protected]>
Co-authored-by: Pratik Prajapati <[email protected]>
Co-authored-by: Ben Blanchard <[email protected]>
Co-authored-by: shpwe <[email protected]>
Co-authored-by: Yagnik Hingrajiya <[email protected]>
Co-authored-by: Zack Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors in data export, Payload appears to copy data from Condition and duplicate rows
4 participants