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.

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

Make the export action asynchronous and add notifications #1572

Merged
merged 16 commits into from Feb 21, 2020
Merged

Make the export action asynchronous and add notifications #1572

merged 16 commits into from Feb 21, 2020

Conversation

chvp
Copy link
Member

@chvp chvp commented Dec 9, 2019 •
edited

Closes #1578.
Closes #1633.

@chvp chvp added this to the 3.3 milestone Dec 9, 2019
@chvp chvp added the feature New feature or request label Dec 9, 2019
@pdawyndt pdawyndt changed the title Make the export action asynchronous make the export action asynchronous Dec 9, 2019
@chvp chvp mentioned this pull request Dec 17, 2019
Closed
@chvp chvp force-pushed the feature/async-export branch 2 times, most recently from fa30412 to f52f119 Compare December 18, 2019 08:46
@chvp chvp force-pushed the feature/async-export branch 4 times, most recently from 29dd59d to 6ca5a28 Compare January 8, 2020 09:23
@chvp chvp force-pushed the feature/async-export branch 3 times, most recently from 3122d0b to fd14f99 Compare January 21, 2020 08:59
@chvp chvp changed the title make the export action asynchronous Make the export action asynchronous and add notifications Jan 21, 2020
@chvp chvp force-pushed the feature/async-export branch 2 times, most recently from 1efa9f4 to d7b8eb0 Compare January 21, 2020 09:07
@chvp chvp force-pushed the feature/async-export branch 3 times, most recently from 8451ebe to 7d271f1 Compare February 4, 2020 16:41
@chvp chvp marked this pull request as ready for review February 5, 2020 09:33
@chvp chvp requested review from bmesuere and rien February 5, 2020 09:34
@chvp
Copy link
Member Author

chvp commented Feb 5, 2020

Export overview:
image
View when there is an unfinished export:
image
Extra link in user menu (only shown if user has exports):
image
Bell with icon if user has unread notifications:
image
Notification listing:
image
Bell when user has notifications, but no unread + menu
image

Clicking the notification takes the user to the exports index (and sets the notification to read).

bmesuere
bmesuere requested changes Feb 5, 2020
View reviewed changes
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

minor remark, by renaming everything to export, it becomes less clear what is what when reading the code. eg, the name scoresheet directly implies an overview for course admins.

app/assets/javascripts/notification.ts Show resolved Hide resolved
app/assets/javascripts/notification.ts Outdated Show resolved Hide resolved
app/assets/javascripts/notification.ts Outdated Show resolved Hide resolved
app/assets/javascripts/notification.ts Outdated Show resolved Hide resolved
app/helpers/export_helper.rb Outdated Show resolved Hide resolved
app/policies/notification_policy.rb Outdated Show resolved Hide resolved
app/views/courses/show.html.erb Outdated Show resolved Hide resolved
app/views/notifications/_notifications_table.html.erb Outdated Show resolved Hide resolved
app/views/notifications/destroy.js.erb Outdated Show resolved Hide resolved
@chvp
Copy link
Member Author

chvp commented Feb 6, 2020

minor remark, by renaming everything to export, it becomes less clear what is what when reading the code. eg, the name scoresheet directly implies an overview for course admins.

I'm not sure what you mean by this. As far as I can tell, the word download is used for things related to the scoresheet functionality.

@chvp chvp force-pushed the feature/async-export branch 2 times, most recently from b2135a3 to 698f582 Compare February 6, 2020 08:39
@chvp chvp force-pushed the feature/async-export branch 5 times, most recently from ad88b93 to d412281 Compare February 18, 2020 19:10
@chvp chvp requested a review from bmesuere February 19, 2020 09:43
@chvp chvp requested a review from rien February 19, 2020 17:20
rien
rien approved these changes Feb 19, 2020
View reviewed changes
Copy link
Member

@rien rien left a comment

Choose a reason for hiding this comment

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

Perfect 👍

bmesuere
bmesuere approved these changes Feb 20, 2020
View reviewed changes
app/assets/javascripts/notification.ts Outdated Show resolved Hide resolved
app/assets/stylesheets/components/notification.css.less Show resolved Hide resolved
app/views/layouts/application.html.erb Show resolved Hide resolved
@chvp chvp merged commit bc5948a into develop Feb 21, 2020
@chvp chvp deleted the feature/async-export branch February 21, 2020 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmesuere bmesuere bmesuere approved these changes

@rien rien rien approved these changes

Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
3.3
3 participants
@chvp @rien @bmesuere