Implement bulk export query and abstract bulk methods into own import #2279

saberlynx · 2020-06-30T13:07:40Z

Checklist:

Tests
CHANGELOG Entry

codecov · 2020-06-30T13:50:01Z

Codecov Report

Merging #2279 into master will increase coverage by 0.15%.
The diff coverage is 94.36%.

@@            Coverage Diff             @@
##           master    #2279      +/-   ##
==========================================
+ Coverage   54.73%   54.89%   +0.15%     
==========================================
  Files        1093     1093              
  Lines       26556    26603      +47     
  Branches     7534     7570      +36     
==========================================
+ Hits        14536    14603      +67     
+ Misses      10913    10895      -18     
+ Partials     1107     1105       -2

Impacted Files	Coverage Δ
gsa/src/web/pages/tasks/component.js	`35.55% <ø> (ø)`
gsa/src/web/pages/tasks/listpage.js	`82.71% <82.60%> (+3.70%)`	⬆️
gsa/src/web/entities/bulkactions.js	`81.30% <100.00%> (ø)`
gsa/src/web/graphql/tasks.js	`100.00% <100.00%> (ø)`
gsa/src/web/entities/actions.js	`100.00% <0.00%> (+12.50%)`	⬆️
gsa/src/web/components/icon/exporticon.js	`100.00% <0.00%> (+18.18%)`	⬆️
gsa/src/web/components/icon/tagsicon.js	`100.00% <0.00%> (+20.00%)`	⬆️
gsa/src/web/components/icon/trashicon.js	`100.00% <0.00%> (+20.00%)`	⬆️
gsa/src/web/entities/selection.js	`84.61% <0.00%> (+76.92%)`	⬆️

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 6b783a8...68d6a9b. Read the comment docs.

sarahd93

So we have bulkactions now that contain the BulkTagComponent, useBulkExportEntities and useBulkDeleteEntities, right? There are tests for the BulkTagComponent but I don't see any for useBulkExportEntities and useBulkDeleteEntities. We should test those too if possible.

The task listpage contains a test "should allow to export all displayed tasks". Parts of the test are commented out because the bulk actions were still missing. Shouldn't this work now, since bulk export for tasks is implemented? (Also "should allow to export all displayed tasks" is probably not the best title because bulk delete seems to be tested as well.)

saberlynx · 2020-07-17T10:19:05Z

So we have bulkactions now that contain the BulkTagComponent, useBulkExportEntities and useBulkDeleteEntities, right? There are tests for the BulkTagComponent but I don't see any for useBulkExportEntities and useBulkDeleteEntities. We should test those too if possible.

You're right. If we didn't abstract the functionality from just tasks alone then it would be simple. As such I'm not quite sure how to test these functions. I'll have to think about that a little more. I remember several weeks ago I wasn't able to come up with a way.

The task listpage contains a test "should allow to export all displayed tasks". Parts of the test are commented out because the bulk actions were still missing. Shouldn't this work now, since bulk export for tasks is implemented? (Also "should allow to export all displayed tasks" is probably not the best title because bulk delete seems to be tested as well.)

I don't think it should because stuff like that used to be from gmp, so we can check from the listpage if they are called. Now it's a graphql function. I can try to see if I can put the mock into the listpage test and get rid of the old tests.

sarahd93

I guess for useBulkExportEntities you would "just" need to test the hook with some simple mock functions for exportByFilterFunc and exportByIdsFunc and test if for each selectionType the correct function is called and returns the correct entities. But I realize that this might be a little more tricky than it looks.

Oh of course the listpage tests need to be adapted to graphQL. I think at least for bulk export this should work. Bulk delete is using refetch, though, and that seems to always break the tests, so we might not be able to test that right now.

bjoernricks

For me it looks good. I am just not sure about the error cases mentioned in the code comment.

gsa/src/web/entities/bulkactions.js

gsa/src/web/pages/tasks/listpage.js

Co-authored-by: Björn Ricks <bjoern.ricks@greenbone.net>

sarahd93

The tests look really good, I think we have a pretty good coverage now.

gsa/src/web/entities/__tests__/bulkactions.js

bjoernricks · 2020-08-14T11:45:25Z

gsa/src/web/pages/tasks/listpage.js

+    const [deleteTasksByIds] = useDeleteTasksByIds();
+    const [deleteTasksByFilter] = useDeleteTasksByFilter();


The indentation still seems to be wrong here. Maybe a bug in prettier...

saberlynx changed the title ~~Bulk export~~ Implement bulk export query and abstract bulk methods into own import Jun 30, 2020

saberlynx force-pushed the bulk-export branch 2 times, most recently from d9db157 to fec54d4 Compare July 2, 2020 10:47

saberlynx changed the base branch from hyperion to master July 2, 2020 10:53

saberlynx marked this pull request as ready for review July 2, 2020 11:43

saberlynx requested review from a team and bjoernricks July 2, 2020 11:43

sarahd93 reviewed Jul 17, 2020

View reviewed changes

bjoernricks reviewed Aug 5, 2020

View reviewed changes

gsa/src/web/entities/bulkactions.js Outdated Show resolved Hide resolved

gsa/src/web/pages/tasks/listpage.js Outdated Show resolved Hide resolved

saberlynx force-pushed the bulk-export branch 12 times, most recently from 7dee781 to fc0e187 Compare August 13, 2020 10:45

saberlynx marked this pull request as draft August 13, 2020 11:20

saberlynx force-pushed the bulk-export branch 5 times, most recently from 12eb532 to 7542f95 Compare August 13, 2020 15:55

saberlynx and others added 17 commits August 14, 2020 09:31

Move bulk delete logic to bulkactions.js

6a34e49

Reorganize and comment task listpage

8c7eea5

Rename export entities

653917d

Implement bulk export entities by ids

ec497cf

Change entitiesType name to resourceType

b3d7433

Test export functions

ae68fa9

Bulk export by ids test

ff75de7

Abstract away task entity type in bulk export tests

43b6d63

Test bulk export entities for all export scenarios

ca292d6

Add bulk delete tests

94ced69

Remove safety checks in bulkExport

cd74c2f

Update gsa/src/web/pages/tasks/listpage.js

6e24c60

Co-authored-by: Björn Ricks <bjoern.ricks@greenbone.net>

Fix wrong onDeleted refetch query and test bulk action on page contents

a9a22b3

Test bulk action by ids

7c76d94

Mark loadAlerts as non required in task component to fix warning

ba536bb

Test bulk action on filtered tasks

8f354c9

Allow passing filterString as arguments to filter querymocks

76b7373

saberlynx force-pushed the bulk-export branch from eb07a5b to cb515e9 Compare August 14, 2020 07:32

Remove checkbox data testid and use the correct one

45a0360

saberlynx force-pushed the bulk-export branch from cb515e9 to 45a0360 Compare August 14, 2020 07:39

Comment code

cdb0023

saberlynx force-pushed the bulk-export branch from 8677435 to cdb0023 Compare August 14, 2020 07:43

saberlynx marked this pull request as ready for review August 14, 2020 07:45

saberlynx requested review from bjoernricks and sarahd93 August 14, 2020 07:46

sarahd93 reviewed Aug 14, 2020

View reviewed changes

gsa/src/web/entities/__tests__/bulkactions.js Show resolved Hide resolved

gsa/src/web/entities/__tests__/bulkactions.js Show resolved Hide resolved

gsa/src/web/entities/__tests__/bulkactions.js Show resolved Hide resolved

Apply review suggestions and clear mock call count after each test

68d6a9b

saberlynx requested a review from sarahd93 August 14, 2020 11:02

bjoernricks approved these changes Aug 14, 2020

View reviewed changes

saberlynx merged commit a56841e into greenbone:master Aug 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement bulk export query and abstract bulk methods into own import #2279

Implement bulk export query and abstract bulk methods into own import #2279

saberlynx commented Jun 30, 2020 •

edited

Loading

codecov bot commented Jun 30, 2020 •

edited

Loading

sarahd93 left a comment

saberlynx commented Jul 17, 2020 •

edited

Loading

sarahd93 left a comment

bjoernricks left a comment

sarahd93 left a comment

bjoernricks Aug 14, 2020

		const [deleteTasksByIds] = useDeleteTasksByIds();
		const [deleteTasksByFilter] = useDeleteTasksByFilter();

Implement bulk export query and abstract bulk methods into own import #2279

Implement bulk export query and abstract bulk methods into own import #2279

Conversation

saberlynx commented Jun 30, 2020 • edited Loading

codecov bot commented Jun 30, 2020 • edited Loading

Codecov Report

sarahd93 left a comment

Choose a reason for hiding this comment

saberlynx commented Jul 17, 2020 • edited Loading

sarahd93 left a comment

Choose a reason for hiding this comment

bjoernricks left a comment

Choose a reason for hiding this comment

sarahd93 left a comment

Choose a reason for hiding this comment

bjoernricks Aug 14, 2020

Choose a reason for hiding this comment

saberlynx commented Jun 30, 2020 •

edited

Loading

codecov bot commented Jun 30, 2020 •

edited

Loading

saberlynx commented Jul 17, 2020 •

edited

Loading