-
Notifications
You must be signed in to change notification settings - Fork 14
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
Feature: Experiment Archive State #987
Conversation
…into feature/archive-status-backend-issue30
Can we get some more technical overview of the changes that were made to aid review and know how to test? If there is any finalized document that goes over the actual requirements that were decided on after all the years of discussion, can someone point to it? |
Sure. So we do have a document. Requirements:
|
Thanks Pratik, that all sounds exactly right. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone over this and it all looks good to me.
Thanks @bcb37 for the review, you can also approve this and it would't auto merge as I have disabled the option as Dan had also pointed out in slack. We can wait for multiple approvals before merging if needed. |
...rade/src/app/features/dashboard/home/components/experiment-list/experiment-list.component.ts
Outdated
Show resolved
Hide resolved
Just a few small comments from me, there's just the one spot in |
backend/packages/Upgrade/src/api/controllers/QueryController.ts
Outdated
Show resolved
Hide resolved
frontend/projects/upgrade/src/app/core/analysis/store/analysis.actions.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frontend doesn't need to know that we are storing the query result of the archive state in a different way. We can use the same endpoint to serve the result.
@VivekFitkariwala Yes, we can merge analysis and achieve branch by giving one more variable to identify which call is sent. I think we made it a different API call because service file functions were very diffenent but now can be manage by if-else. |
The changes look good. |
This PR contains Archived status feature.