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

Add feedback button #527

Merged
merged 65 commits into from Mar 20, 2024
Merged

Add feedback button #527

merged 65 commits into from Mar 20, 2024

Conversation

Splines
Copy link
Member

@Splines Splines commented Jul 4, 2023

New feedback button for users to give feedback regarding MaMpf. Closes #466

Merge #537 beforehand, then bring in mampf-next into this branch.

TODOs

  • Prevent sending feedback with empty message
  • Make sure everything works when title is empty (title is optional)
  • Implement better user flow: close modal and clear message upon successful sending, otherwise show flash error message.
  • Set FEEDBACK_EMAIL env variable in production! -> won't be reflected in this PR, manual in production environment
  • Test email delivery in experimental.

Preview

image
image
image

For reviewers

  • Check internationalization: everything should work fine in German as well as English including
    • all the texts in the modal
    • success/error toast messages
  • Check user flow
    • good path: write feedback and submit, check in Mailcatcher if email arrived
    • error paths: write feedback with no characters, in app/controllers/FeedbacksController inside the create method, add: @feedback_success = false right before respond_to(&:js) to simulate unsuccessful saving of message to database, simulate bad internet connection, other error paths?
  • Code quality & linting!
  • Check that "Allow us to contact you via email" is correctly reflected in Reply-to of mail (only if checkbox is ticked should we be able to contact the user by replying to the mail, otherwise Reply-to field should be empty)
  • Clean state: after having successfully submitted feedback, modal should close and next time have texts cleared. If error: modal should stay open and text should not be cleared. Text should also not be cleared when user writes feedback, closes modal (without submitting), then opens it again.
  • Mobile: test that feedback submit button is still visible in navbar.
  • Own ideas to test this feature: ... (specify)

@Splines Splines self-assigned this Jul 4, 2023
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (dev@469bb11). Click here to learn what that means.

❗ Current head 07ba509 differs from pull request most recent head a36cf52. Consider uploading reports for the commit a36cf52 to get more accurate results

Additional details and impacted files
@@          Coverage Diff           @@
##             dev     #527   +/-   ##
======================================
  Coverage       ?   66.52%           
======================================
  Files          ?      312           
  Lines          ?     9425           
  Branches       ?        0           
======================================
  Hits           ?     6270           
  Misses         ?     3155           
  Partials       ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Splines Splines requested a review from Frodo161 August 22, 2023 14:30
@Splines Splines marked this pull request as ready for review August 22, 2023 14:30
@Splines
Copy link
Member Author

Splines commented Aug 22, 2023

Hey @Frodo161, this PR is now ready for your review ;) Please see the For reviewers section in my upmost comment here. You can tick off the checklist there and also edit my comment.

Copy link
Collaborator

@Frodo161 Frodo161 left a comment

Choose a reason for hiding this comment

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

Problems:

  • "Bitte verwenden Sie zumindest 10 Zeichen" is in German even if the language is set to English.
  • If one uses "@feedback_sucess = false" as described in the PR, nothing happens (only the modal won't close on save). Especially, there is no error message (should there be one?). The mail arrives in the mail catcher as expected.
  • More generally, I didn't get to see any of the error messages that are written down in the locale files.

Code:

  • Maybe, instead of writing authorize_resource except: [:create] in the FeedbacksController, you can add an ability file for feedback.

fosterfarrell9
fosterfarrell9 previously approved these changes Jan 4, 2024
Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 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 to me. Before merging into production: Has this

  • Set FEEDBACK_EMAIL env variable in production! -> won't be reflected in this PR, manual in production environment

been done yet?

* 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?`
* 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 Splines changed the base branch from mampf-next to dev January 25, 2024 18:58
@Splines Splines dismissed fosterfarrell9’s stale review January 25, 2024 18:58

The base branch was changed.

@Splines
Copy link
Member Author

Splines commented Mar 20, 2024

Will merge without approval as Denis is on vacation.

@Splines Splines merged commit 4e031a6 into dev Mar 20, 2024
3 checks passed
@Splines Splines deleted the feature/feedback-button branch March 20, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants