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.

Sign up for GitHub

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

Jump to bottom

storing default condition payload values in db with conditions #1309

Merged
merged 5 commits into from
Feb 26, 2024
Merged

storing default condition payload values in db with conditions #1309

merged 5 commits into from
Feb 26, 2024

Conversation

Yagnik56
Copy link
Collaborator

This PR will address issue of default condition payload not updating in db after changing it by default condition payload values same as condition code getting saved in db.

@Yagnik56 Yagnik56 self-assigned this Feb 14, 2024
@Yagnik56 Yagnik56 linked an issue Feb 14, 2024 that may be closed by this pull request
Updating condition payload to default condition names will not store that in the DB #1297
Closed
@Yagnik56 Yagnik56 requested review from ppratikcr7 and bcb37 February 14, 2024 14:53
@Yagnik56 Yagnik56 requested a review from danoswaltCL February 14, 2024 15:15
ppratikcr7
ppratikcr7 reviewed Feb 14, 2024
View reviewed changes
Copy link
Collaborator

@ppratikcr7 ppratikcr7 left a comment

Choose a reason for hiding this comment

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

Just one small comment, rest looks good.

...projects/upgrade/src/app/core/experiment-design-stepper/experiment-design-stepper.service.ts Outdated Show resolved Hide resolved
@Yagnik56 Yagnik56 requested a review from ppratikcr7 February 15, 2024 05:18
ppratikcr7
ppratikcr7 previously requested changes Feb 15, 2024
View reviewed changes
Copy link
Collaborator

@ppratikcr7 ppratikcr7 left a comment

Choose a reason for hiding this comment

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

@Yagnik56 For Simple experiment, all looks good. But, for factorial experiment, new rows are added for editing of condition payloads instead of overwriting the existing one's.

@Yagnik56 Yagnik56 requested a review from ppratikcr7 February 16, 2024 09:10
VivekFitkariwala
VivekFitkariwala requested changes Feb 16, 2024
View reviewed changes
package.json Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@danoswaltCL
Copy link
Collaborator

At UpGrade root dir, run my custom script npm run version:patch. It looks like maybe you've run the regular npm command npm version patch at the project level, but we'll want all the package.jsons pipped at the same time, so we should see all the package versions changed (and the POM.xml).

@Yagnik56
Copy link
Collaborator Author

Yagnik56 commented Feb 21, 2024 •
edited
Loading

At UpGrade root dir, run my custom script npm run version:patch. It looks like maybe you've run the regular npm command npm version patch at the project level, but we'll want all the package.jsons pipped at the same time, so we should see all the package versions changed (and the POM.xml).

getting some kind of error with that code, could you try it in branch and see if any changes are there to push.

@danoswaltCL
Copy link
Collaborator

Ok don't worry about it, I can bump the versions when I merge.

I pulled and don't get any errors but maybe local setup matters, can you share the errors?

danoswaltCL
danoswaltCL previously requested changes Feb 22, 2024
View reviewed changes
Copy link
Collaborator

@danoswaltCL danoswaltCL left a comment •
edited
Loading

Choose a reason for hiding this comment

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

[deleted comment]

@danoswaltCL danoswaltCL dismissed their stale review February 22, 2024 17:07

tester error

@Yagnik56 Yagnik56 requested a review from danoswaltCL February 23, 2024 05:35
@danoswaltCL danoswaltCL dismissed ppratikcr7’s stale review February 26, 2024 17:58

seems to have been addressed

@danoswaltCL danoswaltCL merged commit 35c9c6b into release/v5.1 Feb 26, 2024
8 checks passed
@danoswaltCL danoswaltCL deleted the storing-default-condition-payload-in-db branch February 26, 2024 18:20
@danoswaltCL danoswaltCL mentioned this pull request Feb 26, 2024
Merged
danoswaltCL added a commit that referenced this pull request Feb 27, 2024
@danoswaltCL @Yagnik56
storing default condition payload values in db with conditions (#1309) (
5e4fb0b 
#1321)

* storing default condition payload values in db with conditions

* version bump to v5.1.1

* code clean up review cmt

* removing old condition payload when new are added

* version bump to 5.1.1

---------

Co-authored-by: Yagnik Hingrajiya <[email protected]>
danoswaltCL added a commit that referenced this pull request Mar 15, 2024
@danoswaltCL @Yagnik56 @ppratikcr7
Merge down bugfixes release 5 1 3 (#1367)
de3abad 
* storing default condition payload values in db with conditions (#1309)

* storing default condition payload values in db with conditions

* version bump to v5.1.1

* code clean up review cmt

* removing old condition payload when new are added

* version bump to 5.1.1

---------

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

* Bugfix/1365 assignedcondition null mark issue (#1366)

* proper null coalescing in mark call for assignedcondition

* make assignedCondition an optional value in MarkData object, as we can safely as assume null is the intended condition

* version bump

* 🐛 Hotfix: Fix for Exclusions logic(Simple & Within Subject Exps) and code optimization for mark call (#1324)

* bugfixes for exclusions logic and mark call opt

* removing extra call to fetch experiment

* Storing proper group exclusion and individual exclusion documents (#1343)

* correct group exclusion and individual exclusion docs with code optimization for assign call

* code optimizations for mark and assign call for exclusions

* resolved peer review comments for group exclusions bug

* version bump release branch

---------

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

---------

Co-authored-by: Yagnik Hingrajiya <[email protected]>
Co-authored-by: Pratik Prajapati <[email protected]>
danoswaltCL added a commit that referenced this pull request May 13, 2024
@ppratikcr7 @Yagnik56
@danoswaltCL @bcb37 @zackcl
✨ Merge release/v5.1 to dev (#1526)
489a738 
* storing default condition payload values in db with conditions (#1309)

* storing default condition payload values in db with conditions

* version bump to v5.1.1

* code clean up review cmt

* removing old condition payload when new are added

* version bump to 5.1.1

---------

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

* Bugfix/1365 assignedcondition null mark issue (#1366)

* proper null coalescing in mark call for assignedcondition

* make assignedCondition an optional value in MarkData object, as we can safely as assume null is the intended condition

* version bump

* 🐛 Hotfix: Fix for Exclusions logic(Simple & Within Subject Exps) and code optimization for mark call (#1324)

* bugfixes for exclusions logic and mark call opt

* removing extra call to fetch experiment

* Storing proper group exclusion and individual exclusion documents (#1343)

* correct group exclusion and individual exclusion docs with code optimization for assign call

* code optimizations for mark and assign call for exclusions

* resolved peer review comments for group exclusions bug

* version bump release branch

---------

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

* Bugfix/1382 experiment type in assign response (#1384)

* add experimentType to IExperimentAssignmentv5 response

* changes to main.java

* remove extra ts lib experimentType logic

* spec update

* spec update dataservice

* version bump

* ✨ Enhancement: Update data CSV export format (#1378)

* bugfixes for exclusions logic and mark call opt

* correct group exclusion and individual exclusion docs with code optimization for assign call

* code optimizations for mark and assign call for exclusions

* updating data csv export format to capture each mark call

* resolve peer review comments to confirm workingGroup is defined

* fix for integration test cases with old DP keys

* use new relic env var for prod and staging (#1438)

* Bugfix/use new relic var (#1442)

* use new relic env var for prod and staging

* version bump 5.1.5

* switch default user role from cretor to reader (#1448)

* experiment list context chip issue is resolved

* send whole url string in email link (#1464)

* send whole url string in email link

* change version to 5.1.7 for pipeline

* snackbar for import and delete experiment (#1468)

* snackbar issue resolved for import and delete operation of experiment

* version bump

---------

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

* Bugfix for consistent metrics statistics view (#1467)

* bugfix for metrics statistics view

* metrics consistent dictionary usage across stepper and details page

* bump to 5.1.9

---------

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

* Merge down release hotfixes 5.0 into 5.1 (#1470)

* cherry-pick ea63855 (#1159)

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

* Hotfix/enrollment complete fix (#1161)

* cherry-pick ea63855

* bump version after enrollment complete  hotfix

---------

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

* Fix/version root only (#1163)

* revert, change version back to major-minor on root only

* add backend

* Fix/version root only (#1164)

* revert, change version back to major-minor on root only

* add backend

* commit the backend package.json

* no assignedCondition null in java lib (#1441)

* fix missing imports

* version bump 5.1.10

---------

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

* peer review comments to improve enrollment code testcases

* Resolved review comment on PR

* Disabled dp and condition table and Allow payload edit (#1473)

* disabled dp and condition table edit and allowed change in condition payload while enrolling

* same changes for factorial experiment

* removed unnecessary the code change

* Grey out the decision points and conditions/factors tables

* Revert "Grey out the decision points and conditions/factors tables"

This reverts commit 91c7494.

* Grey out the decision points and conditions/factors tables (recommit)

---------

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

* add prefix to keys for cache lookup (#1477)

* add prefix to keys for cache lookup

* version bump

* ✨ Toggle for within-subjects experiment type support (#1471)

* toggle for within-subjects experiment type support

* version bump

---------

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

* 🐛 Bugfix to overwrite monitored document for unused decision points (#1482)

* bugfix to overwrite monitored document for unused decision points

* unit test cases fixed for multiple monitored document getting stored for unused dp

* version bump

---------

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

* ✨ Detailed Integration Testcases: Exclusion codes (#1433)

* detailed integration test cases for exclusion codes

* peer review comments to improve exclusion code testcases

* added missing mock experiments while resolving conflicts

* review comments fixed for enrollment code and other integration test cases

---------

Co-authored-by: Yagnik Hingrajiya <[email protected]>
Co-authored-by: danoswaltCL <[email protected]>
Co-authored-by: Yagnik <[email protected]>
Co-authored-by: Ben Blanchard <[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
Reviewers

@danoswaltCL danoswaltCL danoswaltCL approved these changes

@VivekFitkariwala VivekFitkariwala VivekFitkariwala left review comments

@bcb37 bcb37 Awaiting requested review from bcb37

@ppratikcr7 ppratikcr7 Awaiting requested review from ppratikcr7

Assignees

@Yagnik56 Yagnik56

Labels
bugfix release 5.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Updating condition payload to default condition names will not store that in the DB
4 participants
@Yagnik56 @danoswaltCL @VivekFitkariwala @ppratikcr7