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

Ticket7225 #1504

Merged
merged 10 commits into from
Nov 28, 2022
Merged

Ticket7225 #1504

merged 10 commits into from
Nov 28, 2022

Conversation

NikolaRoev
Copy link

@NikolaRoev NikolaRoev commented Nov 16, 2022

Description of work

Separated component groups from configuration groups.

Ticket

ISISComputingGroup/IBEX#7225

Acceptance criteria

See ticket.

Unit tests

Added a test to see if component groups are correctly added to an EditableConfiguration.

System tests

Mention any automated tests or manual tests that you have added or modified, if applicable. The aim is provide information to help the reviewer

Documentation

Highlight and provide a link to any additions or changes to the documentation, if applicable. The aim is provide information to help the reviewer


Code Review

Final Steps

@NikolaRoev NikolaRoev self-assigned this Nov 16, 2022
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.

  • Please make sure you are adding unit tests for new logic - I can't see any as part of this PR. Did you forget to commit them?

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 17, 2022
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 18, 2022
@Tom-Willemsen
Copy link
Contributor

I have two components - comp_a and comp_b - each defines a block and a group.

But it looks like if I add those components to my config, the GUI looks like it would let me add comp_a_block to a different group (but doesn't actually work on save).

I think we need to filter out blocks defined in components from the config-level group editor

image

(please also add tests for these cases)

@Tom-Willemsen
Copy link
Contributor

For blocks not in a group the current code adds a fake "NONE" group, I think this needs filtering out:

image

@Tom-Willemsen Tom-Willemsen merged commit ac6cc0c into master Nov 28, 2022
@Tom-Willemsen Tom-Willemsen deleted the Ticket7225_no_add_block_to_component_group branch November 28, 2022 15:08
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.

2 participants