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

Add a tool to bulk move prepartions from one node to another #4682

Merged
merged 24 commits into from May 21, 2024

Conversation

CarolineDenis
Copy link
Contributor

@CarolineDenis CarolineDenis commented Mar 25, 2024

Fixes #4042

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

  • Select a storage node that contains 1 or more preparations

  • Click "Move Items" (Truck icon)

  • Select the new location to move the items to

  • Click "Move items from {Current Location} here" in the toolbar

  • Verify items are moved

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

front-end changes thus far look good

specifyweb/specify/tree_views.py Outdated Show resolved Hide resolved
@CarolineDenis CarolineDenis added this to the 7.9.6 milestone Mar 28, 2024
Copy link
Member

@maxpatiiuk maxpatiiuk 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, though someone should review the back-end code

specifyweb/specify/tree_views.py Outdated Show resolved Hide resolved
@CarolineDenis CarolineDenis requested a review from a team March 29, 2024 16:47
specifyweb/specify/tree_views.py Outdated Show resolved Hide resolved
specifyweb/specify/tree_views.py Outdated Show resolved Hide resolved
specifyweb/specify/tree_views.py Outdated Show resolved Hide resolved
specifyweb/specify/urls.py Outdated Show resolved Hide resolved
specifyweb/specify/tree_views.py Outdated Show resolved Hide resolved
specifyweb/specify/tree_extras.py Show resolved Hide resolved
specifyweb/specify/tree_extras.py Outdated Show resolved Hide resolved
specifyweb/specify/tree_extras.py Outdated Show resolved Hide resolved
@CarolineDenis CarolineDenis marked this pull request as ready for review April 16, 2024 14:31
CarolineDenis and others added 2 commits April 19, 2024 05:38
Co-authored-by: Jason Melton <64045831+melton-jason@users.noreply.github.com>
Co-authored-by: Jason Melton <64045831+melton-jason@users.noreply.github.com>
@CarolineDenis
Copy link
Contributor Author

@specify/dev-testing @specify/ux-testing
Would it be beneficial to include the ids of the preparations which were bulk moved from the source node to the target node in the audit log entry for the Bulk Move operation?

Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

Looking good! Alongside awaiting feedback for #4682 (comment), just some more auditing code to consider.

The Auditlog on the frontend has special code to make the format more readable for users.

Can you add the bulk move audit log action to this array?

const auditLogActions = [
commonText.create(),
commonText.update(),
commonText.delete(),
queryText.treeMerge(),
queryText.treeMove(),
queryText.treeSynonymize(),
queryText.treeDesynonymize(),
] as const;

The index of each element in this array corresponds to the auditcode on the backend (in this file: https://github.com/specify/specify7/blob/production/specifyweb/specify/auditcodes.py)

Thus, be careful not to change the ordering of any elements and insert the bulkMove action at index 7 (after tree desynonymize).

This will make it so that the bulk move action can be selected in the Action picklist of an Spauditlog query, and that in the query results, the frontend maps the audit code to a much more readable string instead of 7.

Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Select a storage node that contains 1 or more preparations

  • Click "Move Items" (Truck icon)

  • Select the new location to move the items to

  • Click "Move items from {Current Location} here" in the toolbar

  • Verify items are moved

Looks good, items moved to the new location (I love the truck icon as well)! I would like Grant to review this one though to make sure this is the expected behavior

Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

🥳

@CarolineDenis CarolineDenis removed the request for review from grantfitzsimmons May 21, 2024 17:24
@CarolineDenis CarolineDenis merged commit 499e2a8 into production May 21, 2024
7 of 9 checks passed
@CarolineDenis CarolineDenis deleted the issue-4042 branch May 21, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Bulk move preparation locations in the storage tree
4 participants