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

feat: 'Only Mine' toggle for execution tasks list #335

Closed
wants to merge 19 commits into from

Conversation

olga-union
Copy link
Contributor

@olga-union olga-union commented Mar 18, 2022

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Description

Closed in favor of #343

Added Only my executions filter, removed old filter, removed unused fields on the FilterState, updated UserProfile type field to match API data object. Added tests, updated storybook.

Demos

https://user-images.githubusercontent.com/101579322/159072610-f54bc16d-688b-4a64-bc04-5dd3339ca4fd.mov
https://user-images.githubusercontent.com/101579322/159072616-156479d1-99d8-4e73-8df5-ff5e083531c4.mov
https://user-images.githubusercontent.com/101579322/159072621-d2a056ba-891d-4c0d-b527-fd74339ed35f.mov

@welcome
Copy link

welcome bot commented Mar 18, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Copy link
Contributor

@anrusina anrusina left a comment

Choose a reason for hiding this comment

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

Looks good 🥳, haven't performed smoke test yet.
Let me know when you will be ready for final review

Comment on lines 60 to 64
onlyMyExecutionsFilterState?: {
onlyMyExecutionsValue: boolean;
isFilterDisabled: boolean;
onOnlyMyExecutionsFilterChange: (filterOnlyMyExecutions: boolean) => void;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: create a separate type/interface for onlyMyState, to use here as:
onlyMyExecutionsFilterState?: OnlyMyState; for better readability

Olga Nad and others added 2 commits March 21, 2022 15:30
Signed-off-by: olga-union <olga@union.ai>
Signed-off-by: olga-union <olga@union.ai>
@olga-union olga-union force-pushed the olga-union/add-only-my-executions-filter branch from 3cf1f96 to 816f1ec Compare March 21, 2022 20:31
@olga-union olga-union changed the title feat: 'Only Mine' toggle for execution tasks list [WIP] feat: 'Only Mine' toggle for execution tasks list Mar 21, 2022
Comment on lines +14 to +25
describe.each`
isFilterDisabled | initialValue | expected
${undefined} | ${undefined} | ${{ isFilterDisabled: false, onlyMyExecutionsValue: false }}
${false} | ${undefined} | ${{ isFilterDisabled: false, onlyMyExecutionsValue: false }}
${true} | ${undefined} | ${{ isFilterDisabled: true, onlyMyExecutionsValue: false }}
${undefined} | ${false} | ${{ isFilterDisabled: false, onlyMyExecutionsValue: false }}
${undefined} | ${true} | ${{ isFilterDisabled: false, onlyMyExecutionsValue: true }}
${false} | ${false} | ${{ isFilterDisabled: false, onlyMyExecutionsValue: false }}
${false} | ${true} | ${{ isFilterDisabled: false, onlyMyExecutionsValue: true }}
${true} | ${false} | ${{ isFilterDisabled: true, onlyMyExecutionsValue: false }}
${true} | ${true} | ${{ isFilterDisabled: true, onlyMyExecutionsValue: true }}
`('for each case', ({ isFilterDisabled, initialValue, expected }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially wrote this up as individual tests, but thought this may be a good use case to digest them as a table. I have the original code handy, so wouldn't be a problem to revert, if it's just my taste :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any particular taste, but curious how it will look like if one of them fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anrusina only failed cases will show up, like in this screenshot, test case title is customized on line 26, and I just stringified expected to show value instead of [Object Object]
Screen Shot 2022-03-21 at 5 42 53 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good 👍

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #335 (028bf22) into master (24bdaee) will increase coverage by 0.02%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
+ Coverage   65.14%   65.16%   +0.02%     
==========================================
  Files         390      390              
  Lines        8707     8699       -8     
  Branches     1506     1504       -2     
==========================================
- Hits         5672     5669       -3     
+ Misses       3035     3030       -5     
Impacted Files Coverage Δ
src/components/Entities/EntityExecutions.tsx 0.00% <0.00%> (ø)
...c/components/Entities/EntityExecutionsBarChart.tsx 66.66% <ø> (+3.40%) ⬆️
...nts/Executions/filters/useExecutionFiltersState.ts 100.00% <ø> (ø)
src/models/Common/types.ts 100.00% <ø> (ø)
src/components/Executions/ExecutionFilters.tsx 80.43% <100.00%> (+1.36%) ⬆️
...ecutions/filters/useOnlyMyExecutionsFilterState.ts 100.00% <100.00%> (ø)
src/components/Project/ProjectExecutions.tsx 98.14% <100.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b3987a...028bf22. Read the comment docs.

olga-union and others added 14 commits March 22, 2022 13:33
Signed-off-by: Olga Nad <olga@union.ai>
Signed-off-by: olga-union <olga@union.ai>
Signed-off-by: Olga Nad <olga@union.ai>
Signed-off-by: olga-union <olga@union.ai>
Signed-off-by: Olga Nad <olga@union.ai>
Signed-off-by: Olga Nad <olga@union.ai>
Signed-off-by: Olga Nad <olga@union.ai>
* feat: added RawOutputDataConfig in ExecutionSpec
Signed-off-by: Kevin Su pingsutw@apache.org

Signed-off-by: Olga Nad <olga@union.ai>
Signed-off-by: Olga Nad <olga@union.ai>
Signed-off-by: Olga Nad <olga@union.ai>
Signed-off-by: Olga Nad <olga@union.ai>
@olga-union olga-union deleted the olga-union/add-only-my-executions-filter branch March 23, 2022 16:06
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.

4 participants