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

Workshop admins can move enrollments between workshops #30017

Merged
merged 30 commits into from Aug 6, 2019

Conversation

clareconstantine
Copy link

@clareconstantine clareconstantine commented Jul 30, 2019

Workshop admins can now move enrollments from one workshop to another. This means engineer involvement is not necessary in the frequent requests to combine workshops, split up workshops, or even out enrollment between workshops.

PLC-305

No enrollments selected, Move button disabled:
Screen Shot 2019-07-31 at 3 22 46 PM

Enrollment selected, Move button enabled:
Screen Shot 2019-07-31 at 3 22 56 PM

Move enrollments dialog:
Screen Shot 2019-07-30 at 11 25 54 AM

Error message if moving enrollments fails:
Screen Shot 2019-08-02 at 11 20 59 AM

@clareconstantine clareconstantine changed the title Move enrollments Workshop admins can move enrollments between workshops Jul 30, 2019
apps/envConstants.js Outdated Show resolved Hide resolved
@breville
Copy link
Member

UI-wise, I don't think we need to squeeze the dropdown button into a column header. I think it's fine to have it as a normal-sized button outside of the table, probably somewhere up and to the right. Maybe to the right of the download arrow.

@islemaster
Copy link
Contributor

UI-wise, I don't think we need to squeeze the dropdown button into a column header. I think it's fine to have it as a normal-sized button outside of the table, probably somewhere up and to the right. Maybe to the right of the download arrow.

That would also make room for a select-all/deselect-all checkbox at the top of the column.

@agealy
Copy link

agealy commented Jul 30, 2019

UI-wise, I don't think we need to squeeze the dropdown button into a column header. I think it's fine to have it as a normal-sized button outside of the table, probably somewhere up and to the right. Maybe to the right of the download arrow.

That would also make room for a select-all/deselect-all checkbox at the top of the column.

+1! i think that's where we initially planned for the button to go?

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

I'd like to see an additional test confirming that a non-admin can't interact with this route at all.

@codecov-io
Copy link

codecov-io commented Jul 31, 2019

Codecov Report

❗ No coverage uploaded for pull request base (staging@826bd39). Click here to learn what that means.
The diff coverage is 55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             staging   #30017   +/-   ##
==========================================
  Coverage           ?   73.12%           
==========================================
  Files              ?     2049           
  Lines              ?   112131           
  Branches           ?     3399           
==========================================
  Hits               ?    81995           
  Misses             ?    26902           
  Partials           ?     3234
Flag Coverage Δ
#integration 55.06% <ø> (?)
#storybook 56.61% <ø> (?)
#unit 58.12% <18.18%> (?)
Impacted Files Coverage Δ
...llers/api/v1/pd/workshop_enrollments_controller.rb 94.73% <100%> (ø)
dashboard/config/routes.rb 100% <100%> (ø)
dashboard/app/models/ability.rb 100% <100%> (ø)
...ard/components/workshop_enrollment_school_info.jsx 42.73% <18.18%> (ø)

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 826bd39...ca19472. Read the comment docs.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Awesome!

});
} else {
this.setState(state => {
state.selectedEnrollments.push({
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for state to just hold the selected IDs, rather than duplicating this additional enrollment information into it?

@breville
Copy link
Member

breville commented Aug 5, 2019

Would it be worth getting some test coverage of the UI, using UI automation and/or React tests?

@clareconstantine clareconstantine merged commit cffd6d7 into staging Aug 6, 2019
@clareconstantine clareconstantine deleted the move-enrollments branch August 6, 2019 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants