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

Fix the reload behavior of snippets #344

Merged
merged 2 commits into from Jan 16, 2018

Conversation

Projects
None yet
2 participants
@libre-man
Copy link
Collaborator

commented Jan 15, 2018

Description

Make sure snippets are always reloaded when a user tries to expand
snippets. Also don't reload snippets on verify login as this call is done every
time a user reloads a page. Furthermore we don't retry getting snippets if this
call fails, as this not really needed and can lead (and it did) to a infinite
loop of getting snippets if a user does not have the permission to use
snippets. This PR also closes #290 as I can't reproduce it on this branch.

Fix the reload behavior of snippets
Make sure snippets are always reloaded when a user tries to expand
snippets. Also don't reload snippets on verify login as this call is done every
time a user reloads a page. Furthermore we don't retry getting snippets if this
call fails, as this not really needed and can lead (and it did) to a infinite
loop of getting snippets if a user does not have the permission to use
snippets. This PR also closes #290 as I can't reproduce it on this branch.
@codecov

This comment has been minimized.

Copy link

commented Jan 15, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@27ae02e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #344   +/-   ##
=========================================
  Coverage          ?   98.82%           
=========================================
  Files             ?       29           
  Lines             ?     3140           
  Branches          ?        0           
=========================================
  Hits              ?     3103           
  Misses            ?       37           
  Partials          ?        0

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 27ae02e...2b89448. Read the comment docs.

@olmokramer
Copy link
Collaborator

left a comment

#nice

@olmokramer olmokramer merged commit c47b37d into master Jan 16, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@wafflebot wafflebot bot removed the in progress label Jan 16, 2018

@olmokramer olmokramer deleted the fix-snippets-behaviour branch Jan 16, 2018

olmokramer added a commit that referenced this pull request Feb 14, 2018

Fix the reload behavior of snippets (#344)
Make sure snippets are always reloaded when a user tries to expand
snippets. Also don't reload snippets on verify login as this call is done every
time a user reloads a page. Furthermore we don't retry getting snippets if this
call fails, as this not really needed and can lead (and it did) to a infinite
loop of getting snippets if a user does not have the permission to use
snippets. This PR also closes #290 as I can't reproduce it on this branch.

DevinHillenius added a commit that referenced this pull request Mar 5, 2018

Update readme and add new sections to it (#391)
* Make sure assignment title is only updated after submitting (#328)

* Make sure existing users are added to course during BB-zip upload (#327)

* Add the first implementation of TA communication tools (#313)

* Scheduled weekly dependency update for week 00 (#331)

* Update sqlalchemy from 1.1.15 to 1.2.0

* Update werkzeug from 0.13 to 0.14.1

* Revert "Update werkzeug from 0.13 to 0.14.1"

This reverts commit 016ea22.

* Speedup `/assignment/{assignment_id}/submissions/` route (#332)

* Speedup `/assignment/{assignment_id}/submissions/` route

This route was very slow because a very large amount of requests were being
made. This stops sending the `comment` field by default on this route as this
field can become quite large. Furthermore it calculates the grade way more
efficiently if the grade is a rubric grade (otherwise the grade is not
calculated but simply stored in the database) by pre loading the
`selected_items` field on the `Work` objects it also converts the
`max_rubric_points` property to a cached property as a database request is done
to lookup this field.

* Add autocomplete for adding students to a course (#330)

* Add autocomplete for adding students to a course

* Make sure the deadline object is cloned before modification (#333)

* Split can manage course permission (#319)

* Split can_manage_course permission

Instead, we now have these permissions for more fine-grained control:

* can_edit_assignment_info
* can_assign_graders
* can_edit_cgignore
* can_upload_bb_zip
* can_edit_course_roles
* can_edit_course_users
* can_create_assignment

* Add db migration

* Use new permissions in frontend

* Make it possible to migrate without permissions in database

* Parametrise queries in migration

* Add autocomplete for adding students to a course

* Make sure dark mode looks OK

* Add new requirement to requirements.txt

* Make sure rate limiting is also tested

* Make sure limiter is reset before each test function

* Add a new way to check if a user has certain permissions

This way we are able to make sure calling `hasPermission` twice will always
result in a single http request as the http requests are cached as well.

* Revert adding a new javascript dependency

This dependency is not used after all as caching was implemented by hand as this
is more useful for our use case.

* Fix wrong error message when search string is not large enough

* Fix issues pointed out by @olmokramer during review

* Remove conditional container

This makes loading the manage course page way faster.

* Change the way the permission routes work

This makes it way easier to implement a lot of functions. This also corresponds
better to the way the permissions routes were being used by the front end.

* Replace <b-form-input>s with <input>s

* Correctly clear permissions on logout

* Add registration page (#336)

* Increase the speed of multiple pages (#341)

The following pages or routes should be faster:
- `/api/v1/assignments/` - Fix a `N + 1` query problem for the `MixedWhitespace`
    linter by joining in the initial query.
- `/api/v1/assignments/{assig_id}/submissions/?extended` - This route used the
    `CustomExtendedJSONEncoder` encoder, but as submissions contain two `User`
    fields they were also encoded with a extended format. This was unnecessary
    and slow. The `extended_jsonify` now takes a callback that can be used to
    determine which objects should be encoded using `__extended_to_json__`.
- The submission page - this page contains a multi step process for the first
    load: first we load some data, after this has finished we load some
    more. The amount of steps are reduced from three to two. The first stat
- The submissions page - this page also contained such steps, but they were
    completely removed as they were not needed. This page now also does a
    request less as it doesn't request the course info but instead takes this
    info from the `course` property on the requested assignment object.
- `/api/v1/assignments/{assig_id}/submissions/` (POST) - this request is used
    for uploading blackboard zips. This was quite slow as it was
    syncing (`db.session.add`) every new object to the database, now we only do
    this syncing at the end of the loop. Some queries in the loop are also
    removed, this reduces the amount of queries for a normal sized zip (around
    100) users from 1000 to 500.

This commit also includes improving of the type safety of the `db.session.query`
function call as this was needed to implement these speedups.

* Email graders when their status is reset to not done (#339)

* Email graders when their status is reset to not done

This is not done if they toggle themselves back to not done. Not all options are
implemented just yet, still left to do are
- [x] Uploading a new submission that gets assigned to a grader
- [x] Uploading a blackboard zip

* Make sure graders are mailed when uploading blackboard toggle status

* Set grader to not done if assigned a single submission

* Make sure very large rubrics do not overflow the interface (#343)

* Fix the reload behavior of snippets (#344)

Make sure snippets are always reloaded when a user tries to expand
snippets. Also don't reload snippets on verify login as this call is done every
time a user reloads a page. Furthermore we don't retry getting snippets if this
call fails, as this not really needed and can lead (and it did) to a infinite
loop of getting snippets if a user does not have the permission to use
snippets. This PR also closes #290 as I can't reproduce it on this branch.

* Make the way dividing and assigning works more intuitive (#342)

Make the way dividing and assigning works more intuitive

* Add done grading notification email (#346)

* Make the way dividing and assigning works more intuitive

I tried to explain how assigning and redividing works to somebody and a lot of
small things were unclear. This commit will change that by using the following
starting point: you divide an assignment by assigning submissions, but you do
not assign a submission by dividing. This makes it more logical that the
dividing dialog is not updated when assigning (as assigning does not lead to
dividing). However we should send reminder emails to all graders that are
assigned, not divided. This commit tries to explain this with
`description-popover` elements wherever possible.

* Add initial implementation the done email

This changes the UI for setting reminders, however the backend tests for the API
are not yet updated and as the API was changed in a breaking fashion the tests
will now fail.

* Add tests and documentation for the new features

* Add forgotten new files

* Change the name of the ``change_reminder`` function

The new name better reflects what is actually does

* Fix failing travis tests

Revert the renaming of `send_reminder_mails` to `send_reminder_mail` as the
previous name was better.

* Make sure no other functions use `_send_mail` out during tests

Otherwise checking if mails are send becomes way more difficult and the other
functions are already tested.

* Fix building the documentation

* Use correct tag

* Fixed all issues raised by @olmokramer during review

The things fixed are:
- Leading underscores are removed from the docstrings as they were not present
in the names of the variables anymore.
- The default done grading template was updated to make more sense.

* Create a `cancel` method in the submit button

This makes the code to show a warning in `Notifications.vue` way cleaner. This
might also be useful in the future.

* Improve the error messages

This also creates a descriptive message when no reminder type has been chosen by
the user.

* Fix timezone issues with the reminder type

As the time returned by the server is UTC and not local time this caused the
time to decrease with an hour every time it was submitted. This now always first
parses the time returned by the server using `moment`.

* Fix more typo's as pointed out by @olmokramer

* Rename 'can_update_grading' reminder to 'can_update_course_notification'

This also fixes a bug where permissions were not checked correctly. This also
adds tests for these cases.

* Fix the failing `test_warning_grading_done_email` test (#357)

This was happening because the wrong user was used during testing. This was
fixed by using the `teacher_user`.

* Make it possible to force reset of email when using LTI (#347)

* Make it possible to force reset of email when using LTI

This makes it possible to reset your password even if the initially given email
by the LMS was wrong.

* Add tests for new reset email functionality

This also split the function used for the PATCH login route as this makes it
easier to understand what is going on and also what still needed to be tested.

* Fix issues raised in review by @olmokramer

The issues that are fixed are:
- Remove the period after the button in the popover, this looks better.
- Replace a useless loop with `Element.closest`.
- Fix a typo.
- Remove a `console.log` call.

* Update a old migration to make sure it works with the new data (#360)

* Fix the reset password page (#361)

This was done by adding a single = on this page.

* Fix open icon not always showing correctly (#362)

If the state of an assignment was changed to `open` this was not displayed
correctly by highlighting the correct icon.

* Make sure snippets property is always valid (#363)

Initialize `.snippets` on the store to a valid snippet item to make sure no
errors will happen when adding new snippets. This should fix #290 once and for
all.

* Save snippets on ctrl+enter (#364)

* Fix the notifications dialog on webkit (#366)

The issue was that if the `max-height` or `height` of an input was changed
webkit cleared this input. This component now uses `b-collapse` elements which
do not have these problems.

* Fix assignment state buttons for LTI assignment (#367)

* Fix assignment state buttons for LTI assignment

* Allow python dev versions to fail on Travis

This was done because of this issue: celery/celery#4500

* Edit welcome message of docs

* ADD new structure + content to readme

* ADD technologies used and contributing sections

* Finish readme

* Fix minor errors and add links|

* Fix mistakenly added file

* Fix mistakenly added file

* Fix minor suggestions

* Add email

* Add Erik Kooistra to original team

olmokramer added a commit that referenced this pull request Mar 7, 2018

Update readme and add new sections to it (#391)
* Make sure assignment title is only updated after submitting (#328)

* Make sure existing users are added to course during BB-zip upload (#327)

* Add the first implementation of TA communication tools (#313)

* Scheduled weekly dependency update for week 00 (#331)

* Update sqlalchemy from 1.1.15 to 1.2.0

* Update werkzeug from 0.13 to 0.14.1

* Revert "Update werkzeug from 0.13 to 0.14.1"

This reverts commit 016ea22.

* Speedup `/assignment/{assignment_id}/submissions/` route (#332)

* Speedup `/assignment/{assignment_id}/submissions/` route

This route was very slow because a very large amount of requests were being
made. This stops sending the `comment` field by default on this route as this
field can become quite large. Furthermore it calculates the grade way more
efficiently if the grade is a rubric grade (otherwise the grade is not
calculated but simply stored in the database) by pre loading the
`selected_items` field on the `Work` objects it also converts the
`max_rubric_points` property to a cached property as a database request is done
to lookup this field.

* Add autocomplete for adding students to a course (#330)

* Add autocomplete for adding students to a course

* Make sure the deadline object is cloned before modification (#333)

* Split can manage course permission (#319)

* Split can_manage_course permission

Instead, we now have these permissions for more fine-grained control:

* can_edit_assignment_info
* can_assign_graders
* can_edit_cgignore
* can_upload_bb_zip
* can_edit_course_roles
* can_edit_course_users
* can_create_assignment

* Add db migration

* Use new permissions in frontend

* Make it possible to migrate without permissions in database

* Parametrise queries in migration

* Add autocomplete for adding students to a course

* Make sure dark mode looks OK

* Add new requirement to requirements.txt

* Make sure rate limiting is also tested

* Make sure limiter is reset before each test function

* Add a new way to check if a user has certain permissions

This way we are able to make sure calling `hasPermission` twice will always
result in a single http request as the http requests are cached as well.

* Revert adding a new javascript dependency

This dependency is not used after all as caching was implemented by hand as this
is more useful for our use case.

* Fix wrong error message when search string is not large enough

* Fix issues pointed out by @olmokramer during review

* Remove conditional container

This makes loading the manage course page way faster.

* Change the way the permission routes work

This makes it way easier to implement a lot of functions. This also corresponds
better to the way the permissions routes were being used by the front end.

* Replace <b-form-input>s with <input>s

* Correctly clear permissions on logout

* Add registration page (#336)

* Increase the speed of multiple pages (#341)

The following pages or routes should be faster:
- `/api/v1/assignments/` - Fix a `N + 1` query problem for the `MixedWhitespace`
    linter by joining in the initial query.
- `/api/v1/assignments/{assig_id}/submissions/?extended` - This route used the
    `CustomExtendedJSONEncoder` encoder, but as submissions contain two `User`
    fields they were also encoded with a extended format. This was unnecessary
    and slow. The `extended_jsonify` now takes a callback that can be used to
    determine which objects should be encoded using `__extended_to_json__`.
- The submission page - this page contains a multi step process for the first
    load: first we load some data, after this has finished we load some
    more. The amount of steps are reduced from three to two. The first stat
- The submissions page - this page also contained such steps, but they were
    completely removed as they were not needed. This page now also does a
    request less as it doesn't request the course info but instead takes this
    info from the `course` property on the requested assignment object.
- `/api/v1/assignments/{assig_id}/submissions/` (POST) - this request is used
    for uploading blackboard zips. This was quite slow as it was
    syncing (`db.session.add`) every new object to the database, now we only do
    this syncing at the end of the loop. Some queries in the loop are also
    removed, this reduces the amount of queries for a normal sized zip (around
    100) users from 1000 to 500.

This commit also includes improving of the type safety of the `db.session.query`
function call as this was needed to implement these speedups.

* Email graders when their status is reset to not done (#339)

* Email graders when their status is reset to not done

This is not done if they toggle themselves back to not done. Not all options are
implemented just yet, still left to do are
- [x] Uploading a new submission that gets assigned to a grader
- [x] Uploading a blackboard zip

* Make sure graders are mailed when uploading blackboard toggle status

* Set grader to not done if assigned a single submission

* Make sure very large rubrics do not overflow the interface (#343)

* Fix the reload behavior of snippets (#344)

Make sure snippets are always reloaded when a user tries to expand
snippets. Also don't reload snippets on verify login as this call is done every
time a user reloads a page. Furthermore we don't retry getting snippets if this
call fails, as this not really needed and can lead (and it did) to a infinite
loop of getting snippets if a user does not have the permission to use
snippets. This PR also closes #290 as I can't reproduce it on this branch.

* Make the way dividing and assigning works more intuitive (#342)

Make the way dividing and assigning works more intuitive

* Add done grading notification email (#346)

* Make the way dividing and assigning works more intuitive

I tried to explain how assigning and redividing works to somebody and a lot of
small things were unclear. This commit will change that by using the following
starting point: you divide an assignment by assigning submissions, but you do
not assign a submission by dividing. This makes it more logical that the
dividing dialog is not updated when assigning (as assigning does not lead to
dividing). However we should send reminder emails to all graders that are
assigned, not divided. This commit tries to explain this with
`description-popover` elements wherever possible.

* Add initial implementation the done email

This changes the UI for setting reminders, however the backend tests for the API
are not yet updated and as the API was changed in a breaking fashion the tests
will now fail.

* Add tests and documentation for the new features

* Add forgotten new files

* Change the name of the ``change_reminder`` function

The new name better reflects what is actually does

* Fix failing travis tests

Revert the renaming of `send_reminder_mails` to `send_reminder_mail` as the
previous name was better.

* Make sure no other functions use `_send_mail` out during tests

Otherwise checking if mails are send becomes way more difficult and the other
functions are already tested.

* Fix building the documentation

* Use correct tag

* Fixed all issues raised by @olmokramer during review

The things fixed are:
- Leading underscores are removed from the docstrings as they were not present
in the names of the variables anymore.
- The default done grading template was updated to make more sense.

* Create a `cancel` method in the submit button

This makes the code to show a warning in `Notifications.vue` way cleaner. This
might also be useful in the future.

* Improve the error messages

This also creates a descriptive message when no reminder type has been chosen by
the user.

* Fix timezone issues with the reminder type

As the time returned by the server is UTC and not local time this caused the
time to decrease with an hour every time it was submitted. This now always first
parses the time returned by the server using `moment`.

* Fix more typo's as pointed out by @olmokramer

* Rename 'can_update_grading' reminder to 'can_update_course_notification'

This also fixes a bug where permissions were not checked correctly. This also
adds tests for these cases.

* Fix the failing `test_warning_grading_done_email` test (#357)

This was happening because the wrong user was used during testing. This was
fixed by using the `teacher_user`.

* Make it possible to force reset of email when using LTI (#347)

* Make it possible to force reset of email when using LTI

This makes it possible to reset your password even if the initially given email
by the LMS was wrong.

* Add tests for new reset email functionality

This also split the function used for the PATCH login route as this makes it
easier to understand what is going on and also what still needed to be tested.

* Fix issues raised in review by @olmokramer

The issues that are fixed are:
- Remove the period after the button in the popover, this looks better.
- Replace a useless loop with `Element.closest`.
- Fix a typo.
- Remove a `console.log` call.

* Update a old migration to make sure it works with the new data (#360)

* Fix the reset password page (#361)

This was done by adding a single = on this page.

* Fix open icon not always showing correctly (#362)

If the state of an assignment was changed to `open` this was not displayed
correctly by highlighting the correct icon.

* Make sure snippets property is always valid (#363)

Initialize `.snippets` on the store to a valid snippet item to make sure no
errors will happen when adding new snippets. This should fix #290 once and for
all.

* Save snippets on ctrl+enter (#364)

* Fix the notifications dialog on webkit (#366)

The issue was that if the `max-height` or `height` of an input was changed
webkit cleared this input. This component now uses `b-collapse` elements which
do not have these problems.

* Fix assignment state buttons for LTI assignment (#367)

* Fix assignment state buttons for LTI assignment

* Allow python dev versions to fail on Travis

This was done because of this issue: celery/celery#4500

* Edit welcome message of docs

* ADD new structure + content to readme

* ADD technologies used and contributing sections

* Finish readme

* Fix minor errors and add links|

* Fix mistakenly added file

* Fix mistakenly added file

* Fix minor suggestions

* Add email

* Add Erik Kooistra to original team
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.