-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
chore: relative url for notification links #266
chore: relative url for notification links #266
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Dhanus3133, it looks good.
Here's a few things to improve, see below.
The REST API is returning relative URLs as shown in the following screenshot:
That's unintended, we only want this new behavior in the admin widget, the API may be hosted on a different domain and hence must return the full URL.
Please also add a new unit test which tests the new methods that allow returning the relative URLs.
Watch out also from build failures because of failed QA checks, please read the build output to find out how to run qa checks locally and how to fix common formatting and commit message format issues.
acc534e
to
66700a2
Compare
Updated with just the required notification widget changes, although the QA checks for the CSS files are failing for some reason. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected now, thanks!
Take a look at the comments below for some minor code improvements.
If you can add a selenium browser test for this it would be great, let me know!
openwisp_notifications/static/openwisp-notifications/js/notifications.js
Outdated
Show resolved
Hide resolved
openwisp_notifications/static/openwisp-notifications/js/notifications.js
Outdated
Show resolved
Hide resolved
openwisp_notifications/static/openwisp-notifications/js/notifications.js
Show resolved
Hide resolved
9db0fef
to
2dc4a4b
Compare
2dc4a4b
to
875b419
Compare
@nemesifier, I tried adding Selenium tests, but when the notification tab is clicked, it just loads. When I look into the console, it says it cannot find WS and returns 404 in the network. |
Did you mean "it doesn't load"? |
I meant the WebSocket connection is not found, so in the front end, it just waits, causing the spinning UI. recording.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base test class StaticLiveServerTestCase
does not support websockets.
Here's a rough outline of how we might structure these tests:
from django.contrib.staticfiles.testing import StaticLiveServerTestCase
from channels.testing import ChannelsLiveServerTestCase
class SeleniumTestNotifications(StaticLiveServerTestCase, ChannelsLiveServerTestCase):
# ... etc ...
Please consults these links for more information:
cfb5d69
to
743599f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dhanus3133 I missed the latest updates on this, will review asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @Dhanus3133, I left some comments below, I am doing some quick tests locally to verify some of my comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dhanus3133 I pushed some improvements, feel free to review the changes. Once the build passes I will merge. 👍
Fixes: #249