Migrate unread_comments flag (fix inconsistencies)
#587
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
db/migrate/20240109180000_fix_unread_comments_inconsistencies.rb
Outdated
Show resolved
Hide resolved
db/migrate/20240109180000_fix_unread_comments_inconsistencies.rb
Outdated
Show resolved
Hide resolved
fosterfarrell9
requested changes
Jan 12, 2024
db/migrate/20240109180000_fix_unread_comments_inconsistencies.rb
Outdated
Show resolved
Hide resolved
db/migrate/20240109180000_fix_unread_comments_inconsistencies.rb
Outdated
Show resolved
Hide resolved
db/migrate/20240109180000_fix_unread_comments_inconsistencies.rb
Outdated
Show resolved
Hide resolved
fosterfarrell9
requested changes
Jan 16, 2024
db/migrate/20240116180000_fix_unread_comments_inconsistencies.rb
Outdated
Show resolved
Hide resolved
e.g. update comment, more suitable name for the method etc.
|
@fosterfarrell9 After your approval, I will wait with merging until @chrisflav has updated the script on the server such that the containers rebuild from |
fosterfarrell9
approved these changes
Jan 16, 2024
|
The script and the webhook configuration is updated: |
|
Hey @chrisflav thanks, however the respective branch is called The respective url can still be "https://mampf-dev.mathi.uni-heidelberg.de/" though, just like we have the branch |
|
Okay, changed from |
|
@chrisflav Thanks a lot. However, I've just merged this PR and it did not trigger the redeploy of dev, at least I can't see it in dozzle. Any ideas? |
|
Did you merge here in the github interface? The triggered webhooks all have this branch as head reference, that's why |
|
The webhooks are only triggered on |
|
I did indeed merge via the GitHub interface by pressing on the green "Squash and merge" button. Note that this is the exact same workflow we had before: we squash-merged onto Maybe there's something missing in the webhook settings for the |
|
Its redeploying now, I forgot to reload the webhook configuration and was tricked by Githubs webhook interface: I did not notice that the webhook (actually: all three) was triggered twice. Once with this branch as head ref and once with the |
|
@chrisflav Working now, thanks 🙌 |
Splines
added a commit
that referenced
this pull request
Feb 14, 2024
* Add dummy migration * Implement migration for unread comment flag * Remove unnecessary comment * Declare migration as not idempotent * Use array.length instead of counting * Throw error to prevent revert of migration * Fix severe flaws in unread comments migration * Simplify Reader retrieval * Use the more explicit `.nil?` method * Update migration date * Fix annoying bug: don't use `.select!` but `.select` * Polish migration e.g. update comment, more suitable name for the method etc. * Rename method according to #585
Splines
added a commit
that referenced
this pull request
Feb 15, 2024
Splines
added a commit
that referenced
this pull request
Feb 15, 2024
* Add fix for migration #587 * Mark flawed unread comments migration as stale
fosterfarrell9
added a commit
that referenced
this pull request
Feb 16, 2024
* Warn about too long GitHub commit messages (#586) * Fix comment status (#585) * Reapply first fix for Reader/Media See discussion on #574 for further details. Previous PR for this was #576, closed in favor of this one as this directly branches off the new "dev" branch. * Correctly show latest post (might be current_user's comment) * Fix update of unread comments logic in comments controller * Fix update icon logic and latest post comment * Simplify latest comment logic * Improve code comments * Further improve comments * Fix wording in comment * Fix construction of media array & use `.blank?` instead of `.empty?` * Migrate `unread_comments` flag (fix inconsistencies) (#587) * Add dummy migration * Implement migration for unread comment flag * Remove unnecessary comment * Declare migration as not idempotent * Use array.length instead of counting * Throw error to prevent revert of migration * Fix severe flaws in unread comments migration * Simplify Reader retrieval * Use the more explicit `.nil?` method * Update migration date * Fix annoying bug: don't use `.select!` but `.select` * Polish migration e.g. update comment, more suitable name for the method etc. * Rename method according to #585 * Use `warn` log level for migration (#588) * Rename get_statistics.coffee file to statistics.coffee (#591) This is to reflect the corresponding renaming of the route due to rubocop. * fix behaviour of media search * Disable OR/AND buttons if "all" tags is enabled (inside the media search, right now only frontend change) * Use ids autogenerated by Rails This is such that we can still click on the label to select the respective radio button, instead of only being able to click on the radio button itself. * Disable AND/OR initially (until user deselects "all tags") * Don't restrict media search if "all" tags is enabled. If the "all tags" option is enabled in the search, we should allow the results to include *any* tag. This is automatically the case if we just don't add any restriction regarding the tag ids in the SOLR search. This change accompanies the frontend change to disable the AND/OR buttons if the "all" button is selected meaning the user wants to search for all tags. See #593 for more details. --------- Co-authored-by: Splines <37160523+Splines@users.noreply.github.com> Co-authored-by: Splines <dominic-plein@gmx.de>
Splines
added a commit
that referenced
this pull request
Mar 20, 2024
Due to a forced push of the dev branch, the following list might unfortunately include items from other branches as well. * Init Feedback model * Add Feedback modal view and corresponding controller First working version, of course still a lot to improve from here. * Migrate feedback form to Bootstrap v5 * Add basic styling to Feedback form * Add "allow contact via mail" checkbox A new column was added to the Feedbacks schema. Note that we did not create a new migration as this is a PR which should only contain one migration, namely the one for the creation of the whol Feedback table. * Toggle "allow email contact" by default * Improve submit button handler (outsource to function) * Init feedback mailer Right now just for ourselves, so that we get a plaintext mail with the feedback of a user. Env variables were adjusted accordingly, but need to be set manually in the production environment! * Adjust feedback mail in views * Implement success/error flow with toast messages * Add missing database field "can_contact" * Add internationalization to feedback error/success * Lint some files * Set feedback text field as required with min 10 chars * Add "optional" to title in email * Adjust spacing around feedback button * Internationalize tooltip * Delete console log * Add comment describing hidden submit button handler * Delete default test specs * Add proper validation for Feedback body Alongside this, also made sure that we use a custom client-side validation message when input is too short (under 10 chars long). This allows us to use the language the user has selected in MaMpf instead of the browser language. * Default `can_contact` to false in backend * Update bootstrap to v5.3.1 command used: bundle update bootstrap bundle update bootstrap --conservative did not work, as docker containers did not start again due to dependency errors * Revert "Update bootstrap to v5.3.1" in favor of PR #537 This reverts commit 5cd1af2. * Submit form via Ctrl + Enter when modal is opened * Remove default nil value from ENV.fetch() * Revert "Remove default nil value from ENV.fetch()" This reverts commit 696a395. * Rename button to 'Send' (not 'Save') * Check if should register feedback event handlers * Make feedback button ID more specific * Fix line wrapping (code style) * Use delete on cascade to be able to delete a user even if he/she has sent some feedback * Move Send button before Cancel button * Replace "on delete cascade" with "dependent destroy" * Add cypress rules to ESLint & ignore some patterns * Allow usage of tempusDominus global variable * Ignore JS files with Sprocket syntax * Further improve rules, e.g. allow common globals * Ignore sprocket syntax in cable.js * Autofix all `.js` and `.js.erb` files Command used: `yarn run eslint --fix .` Still 47 problems (27 errors, 20 warnings) after this. * Fix variables in turbolink fix * Prepend unused variables with "_" * Get rid of unused widget variable * Fix specs comment tab alignment * Warn about too long GitHub commit messages (#586) * Fix comment status (#585) * Reapply first fix for Reader/Media See discussion on #574 for further details. Previous PR for this was #576, closed in favor of this one as this directly branches off the new "dev" branch. * Correctly show latest post (might be current_user's comment) * Fix update of unread comments logic in comments controller * Fix update icon logic and latest post comment * Simplify latest comment logic * Improve code comments * Further improve comments * Fix wording in comment * Fix construction of media array & use `.blank?` instead of `.empty?` * Migrate `unread_comments` flag (fix inconsistencies) (#587) * Add dummy migration * Implement migration for unread comment flag * Remove unnecessary comment * Declare migration as not idempotent * Use array.length instead of counting * Throw error to prevent revert of migration * Fix severe flaws in unread comments migration * Simplify Reader retrieval * Use the more explicit `.nil?` method * Update migration date * Fix annoying bug: don't use `.select!` but `.select` * Polish migration e.g. update comment, more suitable name for the method etc. * Rename method according to #585 * Use `warn` log level for migration (#588) * Fix linting in feedback.js * Fix RuboCop errors * Fix remaining ESLint errors * Update timestamp of feedback migration * Add missing Feedback email to prod docker.env * Remove unnecessary Feedback env variables * Add validation message for empty body * Change `const` to `var` to avoid "redefined" errors * Update timestamp of feedback migration (again)
Splines
added a commit
that referenced
this pull request
Mar 31, 2024
* Outsource resize and fullscreen functions for thyme players.
* Outsource chapter functionality of the (normal) thyme player.
* Fix some minor errors and clean up.
* Outsource emergency button to separate file.
* Outsource heatmap functionality to separate JavaScript file.
* Make heatmap a variable of thyme feedback.
* Made some auxiliary functions private.
* Outsource functionaly of the annotation toggle and improve behaviour.
* Outsource some parts of the functionality of thyme which checks if the native HTML player is used or not.
* Improve display manager.
* Outscource video reference into thyme_attributes.js.
* Put element field back into the constructors of buttons such that the ids from the HTML files can be different in different players.
* Remove speed selector from thyme feedback player and adjust some CSS attributes in normal and feedback player.
* Outsource speed selector functionality in separate file.
* Rename update_markers into update_annotations (now also in non-JS-files).
* Remove reference on the video control bar in the java script files as it is not used.
* Outsource global annotation function into a new class called "AnnotationManager".
* Restructure annotation area.
* Fix some properties of the annotation-tool.
* Bring toggle-visible-for-teacher-annotations function (fully) to frontend (Java Script).
* Fix controller method to make the last commit work.
* Rename thyme.js into thyme_player.js.
* Clean up.
* Remove annotationSort() from utility.js as this function is now part of annotation_manager.js.
* Remove function annotationIndex() from utility.js as this function is unused by now.
* Outsource the parts of the thyme player scripts which set up the max time label.
* Code clean up and fix error message for editing annotations.
* Remove comment and made the function sortAnnotations() static.
* Next and previous annotation buttons now consider only valid annotations.
* Move attribute activeAnnotationId to the class AnnotationArea (now save the full annotation instead of just the id).
* Optical and structural improvements of code and fix of updating annotations after submitting the annotation form.
* Rename class Button as Component.
* Outsource hide functionality of the control bar.
* Rename class ControlBar as ControlBarHider and restyle some parts of the code.
* Outsource ia button.
* Outsource ia-close button.
* Replace expressions of the form "$('#' + component.id)" by "$(component)".
* Renamed chapters.js into chapter_manager.js and wrap the functionality into a class.
* Remove function iconClass() as it seems to be used nowhere.
* Made max height of the heatmap a static attribute.
* Outsource chapter functionality into chapter manager class. Remove back button (it'll be added again in a few commits when the chapter manager is well-structured).
* Changed fixed chapters id into an attribute of the chapter manager constructor.
* Remove parameter from nextChapterStart() and previousChapterStart() (current video time can be taken from thymeAttributes).
* Remove chapter class as modelling chapters as objects seems to make the situation more complicated than it is (at least for the moment).
* Remove unused variable from chapter manager.
* Outsource metadata functionality.
* Made metadataBefore() and metadataAfter() private.
* Clean up.
* Add back button again.
* Remove unnecessary variables from thyme editor and fix use of thymeUtility function secondsToTime().
* Change var to let or const in the thyme editor script.
* Add missing ; in for-loop and fix variable declaration.
* Make time in the plus/minus (ten) button variable.
* Replace comment.
* Use play, mute and plus/minus x seconds buttons from the new classes.
* Use seek bar from the new class and fix an event listener.
* Use volume bar from the new class.
* Use setUpMaxTime() from thymeUtility and remove unnecessary time update listener.
* Move dataURLtoBlob() to thymeUtility.
* Outsource add item/reference/screenshot button.
* Rename near_mistake_annotations by num_nearby_mistake_annotations and clean up code.
* Replace find_by_id by find_by.
* Clean up medium model.
* Get rid of unused comment.
* Rename id.
* Change id name.
* Capitalized the German words Du, Deine, etc.
* Fix emergency link feature and made "You need further help? ..." invisible if no link was entered by the teacher.
* Split up thyme CSS.
* Remove global CSS which is not needed.
* Remove duplication of the color map.
* Update annotation area when annotations are updated.
* Fix bad behaviour of ia-button, interactive area and annotation area.
* Replace CoffeeScript by JavaScript in app/views/annotations and clean up. Now, all subcategory radios show the emergency link (before it was only definition).
* Add tooltips for the category radio buttons and for the "post as comment"-field.
* Clean up code.
* Add comment.
* Put thyme player scripts in the thyme folder, separate load of thyme related scripts in application.js and add warning comment.
* Fix missing Latex preview in the annotation modal.
* Lower the opacity of previous/next button in the annotation area if the given annotation ist the first/last of the array.
* Correct language.
* Select necessary annotation attributes in backend, not in frontend.
* Improve interaction between annotation and associated comment.
* Set default value of visible_for_teacher to false, whenever the annotation_status doesn't equal 1 (before it was only set to false if the annotation_status equalled -1).
* Fix wrong/missing annotation subtext in the annotation area.
* Remove warning locales from the form html file and put it into the locales html file.
* Make the annotations in the thyme player appear (uniformly) in the same language as the user language.
* Code clean up.
* Replace text "subtext" by enum "subcategory" in the annotation table in the db.
* Add (sub)category class (+ superclass) to remove hardcoded categories in the (non-view) JavaScript.
* Comment update implies annotation update.
* Change label "Enable emergency button?" and add helpdesk.
* Add case "Thymestamp not found" to update_comments.
* Code clean up.
* Code clean up.
* Fix resize.
* link Commontator::Comment and Annotation models
* Delete unwanted .ruby-version file
* Update pdfcomprezzor to version on mampf-next
* Fix resize and IA bugs.
* Remove "video is not null" check from thyme players.
* Remove test for Internet Explorer
* Set default values for annotations_status
* Update annotations without delay
* Replace plus/minus button by time button.
* Add shortcuts for prev/next annotation.
* Merge together multiple annotation-related migrations
* Improve code comment
* Remove renaming of "this" in JS class
* Redesign "post as comment" checkbox
according to Bootstrap here:
https://getbootstrap.com/docs/5.3/forms/checks-radios/#checks
* Restructure cancel/close buttons
* Improve annotation commontator::comment association
* Code clean up
* Code clean-up
* Replace name "emergency button" by "annotation button"
* Add color check
* Fix comment is nil bug
* Code clean-up
* Add non-null constraint for category
* CSS improvement and bugfix
* Correct locales
* Add null constraint to category in original migration
* Rearrange save button
* Quick fix thyme feedback resize.
* Add non-nil constraint
* Add constraint to AnnotationsController
* Improve code style
* Rename variables to proper camelCase
* Improve code style
* Revert time_stamp.rb
* Remove ==/!= null statements
* Add newline after/before function
* Add comment
* Clean up code/fix bug
* Check timestamp in backend
* Add annotations status check + update
* Fix user authorization
* Reduce complexity of small/large display checks
Also removed deprecated device-width, see:
https://stackoverflow.com/a/18500871/
* Implement locking to prevent unnecessary DB calls
The AnnotationManager should handle its internal state on its own.
Therefore, updateMarkers() now checks if annotations are null
and if they are, it calls updateAnnotations() accordingly,
but only if no other method has already called updateMarkers
recently, e.g. when multiple resize events are fired in a small
period of time. Resource is freed in updateAnnotations.
* Rewrite handling of annotation update
* Use gender-neutral pronoun
* Replace alert() by Bootstrap alert
Used for "Post as comment" warnings.
* Use JQuery syntax and lint file
Note we also use .trigger("change") to trigger event listeners
for the checkbox, otherwise they won't be fired.
* Add tooltip (helpdesk) for category
* Make color picker elements round
* Use paddings instead of line breaks
* Don't show warnings if annotation was posted
* Show further help link in new tab
Also shortened current distinction into different categories
as right now it is not needed.
* Fix unwanted line indentation
* Use bootstrap switch for preview toggle
* Use bootstrap switch for annotation toggle
Also removed unwanted string concatenation artifacts
* Fix alignment of preview column & make more responsive
Note that we did not optimize for very small devices as the annotation tool
cannot be opened there. However, for tablets it should work fine.
* Fix case-sensitive color lookup bug
Due to this bug, one could not create annotations
with the last color (almost white).
* Pause video also when editing an annotation
* Update modal background according to annotation color
When we create a new annotation, a random color is chosen.
TODO: Maybe do this random choice already in the backend,
not just in the frontend.
* Fix another non-capitalized color bug
* Add transition to modal header background color
* Add more restrictive color check
* Don't show toggle if it doesn't do anything
* Put radius to backend
* Add cypress rules to ESLint & ignore some patterns
* Allow usage of tempusDominus global variable
* Ignore JS files with Sprocket syntax
* Further improve rules, e.g. allow common globals
* Ignore sprocket syntax in cable.js
* Autofix all `.js` and `.js.erb` files
Command used:
`yarn run eslint --fix .`
Still 47 problems (27 errors, 20 warnings) after this.
* Fix variables in turbolink fix
* Prepend unused variables with "_"
* Get rid of unused widget variable
* Fix specs comment tab alignment
* Bump active record schema version
This was automatically done when running the pending migrations.
* Run ESLint autofixes
* Hack: Add Thyme & Annotation tool globals to ESLint globals
See the comment in .eslintrc.js for more details for why we do this.
* Rename resize to Resizer to avoid name conflicts
* Show correct modal title.
* Fix duplicate comment bug
* Execute Rubocop safe autocorrect.
* Warn about too long GitHub commit messages (#586)
* Fix safe rubocops (manually)
* Fix unsafe autocorrections
* Fix comment status (#585)
* Reapply first fix for Reader/Media
See discussion on #574 for further details.
Previous PR for this was #576, closed in favor of this one
as this directly branches off the new "dev" branch.
* Correctly show latest post (might be current_user's comment)
* Fix update of unread comments logic in comments controller
* Fix update icon logic and latest post comment
* Simplify latest comment logic
* Improve code comments
* Further improve comments
* Fix wording in comment
* Fix construction of media array & use `.blank?` instead of `.empty?`
* Migrate `unread_comments` flag (fix inconsistencies) (#587)
* Add dummy migration
* Implement migration for unread comment flag
* Remove unnecessary comment
* Declare migration as not idempotent
* Use array.length instead of counting
* Throw error to prevent revert of migration
* Fix severe flaws in unread comments migration
* Simplify Reader retrieval
* Use the more explicit `.nil?` method
* Update migration date
* Fix annoying bug: don't use `.select!` but `.select`
* Polish migration
e.g. update comment, more suitable name for the method etc.
* Rename method according to #585
* Use `.instance_of` (fix unwanted merge artifact)
* Clean up annotation migrations
* Fix wrong table changing in migration & update schema
* Improve annotation area css
* Fix scrollable video bug
* Improve icons in annotation area
* Increase button click area
* Change annotation button icon
* Fix spacing between annotation area buttons
* Add title directly on <a> tags & replace close button
* Fix alignment of video control bar
* Redesign annotation markers (use map pin icon)
* Highlight currently active annotation marker
* Reregister hotkeys also after *edit* modal closes
* Improve styling of annotation button
* Add subtle gradient to video control bar
* Show active annotation marker in front
* Fix ESLint warnings
* Delete duplicate line
* Remove unused variables resp. add underscore
* Delete unnecessary `annotations.coffee` file
* Show new annotation directly in AnnotationArea
* Add shortcut to jump to current annotation
* Make annotation button more prominent
- place it closer to the timeline
- color it with more distinct colors to showcase it
* Show preview by default in annotation modal
* Make delete button id more specific
* Add icons to save/delete in annotation modal
* Improve feedback player (CSS, category toggles etc.)
- CSS: extend from main thyme player instead of copying all CSS styles over
- Register correct shortcut to switch between annotations
- Use bootstrap switches instead of custom-made toggles
- Redesign colors for feedback player category toggles
- Simplify AnnotationCategoryToggle component
* Remove syntax error (duplicated line)
* Remove unnecessary console log
* Fix broken key listeners in feedback player
* Improve positioning of heatmap & toggle spikes as well
* Also show heatmap spikes for "mistake" annotations
* Avoid heatmap being cut off at the sides
* Fix video size in feedback player
* Fix small gap in thyme player resize issue
Instead of jQuery width() and height(), we use
window.innerWidth and window.innerHeight.
* Fix wrong resizing when exiting full screen mode
* Disable annotation key listeners when area closed
* Increase width of timeline in feedback player
Also fixed positioning of other elements on the bottom bar.
* Fix keyboard shortcut abbreviation hint
* Update schema & timestamps for annotation DB migrations
* Ignore other lib paths in autoloading
* Do not load Commontator extension in precompiling
* Get rid of "@extend" in SCSS, use CSS classes instead
* Fix amplitude of heatmap if no annotations are present
* Use string interpolation to construct heatmap string
* Don't rescue `NoDatabaseError`
* Fix nulldb error
* Add "sure to delete" warning in annotation modal
* Change annotation status numbering & activate by default
This is the new labeling:
-1: inherit from lecture
0: disabled
1: enabled
Beforehand it was:
-1: disabled
0: inherit from lecture
1: enabled
However, I think it makes more sense to have 0/1 for disabled/enabled
and leave the -1 for the "inherit" use case.
Also, the "share annotation with lecturer" feature is now enabled by
default for all lectures.
* Rename "Thymestamp" to "Timestamp"
* Update annotation db migration timestamps (one last time)
* Use enum for status in emergency link update
* Prepend emergency link with "https://" if necessary
* Remove unwanted change
---------
Co-authored-by: fosterfarrell9 <28628554+fosterfarrell9@users.noreply.github.com>
Co-authored-by: Splines <37160523+Splines@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates the
unread_commentsfor all users such that it is consistent with theReadermodel again. This is due to #515, which introduced unwanted behavior. The behavior itself is fixed in #585.Useful for reviewers
Useful SQL query for pgadmin:
In your mampf docker container, search for "Ran through" after the migration. Please try out many use cases, e.g. when the flag shouldn't be changed for any user (log should show "Fixed 0 users"), when the flag for two users should be updated etc. Please test this migration thoroughly to avoid nasty things in the production db.