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

Notification rework #501

Merged
merged 16 commits into from
Mar 25, 2019
Merged

Notification rework #501

merged 16 commits into from
Mar 25, 2019

Conversation

chrisma
Copy link
Contributor

@chrisma chrisma commented Mar 23, 2019

  • Buttons for notifications: marking as read and deleting
  • Show datetime of notification, including human-readable format
  • Restrict clickable area of notifications to title
  • When title is clicked, mark notification as read
  • Display only unread notifications on dashboard
  • Write a test that tests clicking on a sudo rights granted notification (i.e. calling mark_as_read_and_redirect) and runs into "too many redirects"

image

@chrisma chrisma added the WIP work is still in progess for this PR label Mar 23, 2019
@chrisma chrisma self-assigned this Mar 23, 2019
@chrisma chrisma added Review Required and removed WIP work is still in progess for this PR labels Mar 23, 2019
@chrisma chrisma changed the title notification index styling and links notification styling and links Mar 23, 2019
@bdaase
Copy link
Contributor

bdaase commented Mar 24, 2019

Clicking on notifications sometimes results in too many redirects for me
image

Sadly, I can not say when it exactly happens...

@chrisma
Copy link
Contributor Author

chrisma commented Mar 24, 2019

Sadly, I can not say when it exactly happens...

Thanks for checking it out. Which type of notification is behaving this way, e.g. 'request accepted', 'role change'? I'm having issues replicating the fault. I don't think the tests cover all types of notifications.

Does the issue still occur if you replace redirect_back fallback_location: notifications_url in app/controllers/notifications_controller.rb with redirect_to dashboard_path?

@bdaase
Copy link
Contributor

bdaase commented Mar 24, 2019

@chrisma It happend with a "Sudo rights granted" notification.
Replacing it seems to fix the issue 👍

@chrisma
Copy link
Contributor Author

chrisma commented Mar 24, 2019

@chrisma It happend with a "Sudo rights granted" notification.
Replacing it seems to fix the issue

While this is a fix, it also diminishes functionality somewhat. I would be interested in a regression test that involves a sudo rights granted notification (and the original code).

@chrisma chrisma added WIP work is still in progess for this PR and removed Review Required labels Mar 24, 2019
@chrisma chrisma mentioned this pull request Mar 24, 2019
@chrisma chrisma changed the title notification styling and links Notification rework Mar 24, 2019
@bdaase
Copy link
Contributor

bdaase commented Mar 24, 2019

@chrisma It happend with a "Sudo rights granted" notification.
Replacing it seems to fix the issue

While this is a fix, it also diminishes functionality somewhat. I would be interested in a regression test that involves a sudo rights granted notification (and the original code).

We should definitely think about it!

@chrisma
Copy link
Contributor Author

chrisma commented Mar 25, 2019

Even with empty links on notifications, I cannot reproduce this error. In the Rails console: User.first.notify('My test notification', 'This totally has a link', 'https://google.com')
User.first.notify('My test notification', 'This does not have a link')

The only weird behavior I found was with external links, e.g. 'https://google.com', in the test environment.

@chrisma
Copy link
Contributor Author

chrisma commented Mar 25, 2019

As I'm currently not able to reproduce the issues, neither manually nor with the automated tests, I'm merging this, so that development can continue with these changes.

However, please open another issue if errors are found.

@chrisma chrisma merged commit 0d77a3d into dev Mar 25, 2019
@chrisma chrisma deleted the notification_display branch March 25, 2019 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP work is still in progess for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants