Skip to content

HEEDLS-509 Added add delegate group page #612

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Sep 23, 2021
Merged

HEEDLS-509 Added add delegate group page #612

merged 11 commits into from
Sep 23, 2021

Conversation

@davidm-m
Copy link
Contributor

JIRA link

HEEDLS-509

Description

Added a new page and data service function to create a new delegate group, and linked the add button on the delegate groups page to it.

Screenshots

image
image
image

Developer checks

I have:

  • Run the formatter and made sure there are no IDE errors.
  • Written tests for the changes (controller, data services, services, view models etc) and manually tested my work with and without JavaScript. Full manual testing guidelines can be found here: https://softwiretech.atlassian.net/wiki/spaces/HEE/pages/6703648740/Testing
  • Updated/added documentation in Swiki and/or Readme.
  • Updated my Jira ticket with information about other parts of the system that were touched as part of the MR and have to be sanity tested to ensure nothing’s broken.
  • Scanned over my own MR to ensure everything is as expected.

ibrahimmunir14
ibrahimmunir14 suggested changes Sep 20, 2021
View reviewed changes
ibrahimmunir14
ibrahimmunir14 approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@ibrahimmunir14 ibrahimmunir14 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

stellake
stellake reviewed Sep 22, 2021
View reviewed changes
stellake
stellake reviewed Sep 23, 2021
View reviewed changes
DigitalLearningSolutions.Data.Tests/Services/GroupsServiceTests.cs Outdated

// Then
result.Should().Be(returnId);
A.CallTo(() => groupsDataService.AddDelegateGroup(A<GroupDetails>._)).MustHaveHappenedOnceExactly();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - should we actually assert that we set group details correctly? (including linkeToField etc defaults)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh good point (although at that point the test will basically be line-for-line recreating the method)

stellake
stellake approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@stellake stellake 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 - just need to change the delegate groups test to assert the GroupDetails are set correctly. No need for rereview though.

@davidm-m davidm-m merged commit 7e13f30 into master Sep 23, 2021
@davidm-m davidm-m deleted the HEEDLS-509-add-new-delegate-group branch September 23, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@stellake stellake stellake approved these changes

@ibrahimmunir14 ibrahimmunir14 ibrahimmunir14 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@davidm-m @stellake @ibrahimmunir14