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

Ticket7515 #1519

Merged
merged 13 commits into from
Feb 17, 2023
Merged

Ticket7515 #1519

merged 13 commits into from
Feb 17, 2023

Conversation

NikolaRoev
Copy link

Ticket

ISISComputingGroup/IBEX#7515

Acceptance criteria

See ticket.


Code Review

Final Steps

@NikolaRoev NikolaRoev self-assigned this Jan 13, 2023
Copy link
Contributor

@Tom-Willemsen Tom-Willemsen left a comment

Choose a reason for hiding this comment

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

  • I think if either the source or the destination server versions start with 0.0.0, then we should consider these "development versions", assume they're up-to-date, and always allow an import (with a suitable warning message about possible version incompatibilities). This would make it much easier for developers to test locally, too.
  • We are leaking various observers / listeners due to not unsubscribing them, and them being created each time the dialog is opened. We either need to wrap these in some kind of singleton, so that they are only created once, or the better solution is to unsubscribe each listener when it's no longer required.
  • It would be nice to be able to select a "custom instrument" manually for testing purposes or in case the instrument list PV is unavailable - just like in the instrument selection dialog, you can put in your own arbitrary PV prefix to change to an instrument not on inst list
  • I think this is complex enough to need a system test. If you can think of a nice way to test with squish, do that, otherwise add a test or two to the manual system tests spreadsheet that exercise this functionality in the same way that a user would.

@pheest pheest merged commit 0d4c52c into master Feb 17, 2023
@NikolaRoev NikolaRoev deleted the Ticket7515_copy_components branch February 17, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants