-
Notifications
You must be signed in to change notification settings - Fork 35
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 centralised link list in the network management #2971
Conversation
Code Climate has analyzed commit bb28096 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 30.7% (50% is the threshold). This pull request will bring the total coverage in the repository to 82.7% (0.0% change). View more on Code Climate. |
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.
I've tested the feature, and it works fine on my local machine. However, I am concerned about the use of time.sleep
calls. In my experience, this is not a reliable practice.
The issue is that the action we're waiting for with time.sleep
can vary in execution time depending on factors like system load, potentially causing failures in production where debugging would be challenging. Additionally, if the underlying action runs within milliseconds, time.sleep
adds unnecessary wait time, causing the user to wait while the system idles.
Is there another way to wait for the signal to finish?
Totally agree 👀 but me neither know any handy way. |
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.
Thanks! this looks mostly good to me.
An additional column for showing the region(s) of the link could be useful, but we can also open another issue for that I guess.
31faaa2
to
4cdb44e
Compare
@david-venhoff @deen13 |
4cdb44e
to
d7b4c48
Compare
d7b4c48
to
f3a4d5b
Compare
208300f
to
7e52851
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.
Awesome, this looks pretty much good to me 🎉
It might be helpful to add a new table column to the network management link list that shows the regions of the link, but maybe that should better be done in a separate pr, as it might be a bit of effort.
I think it would be useful to add some tests for e.g. the bulk actions and the search and replace functionality in network management. But that could of course be done in a separate pr.
cd9add3
to
78c6476
Compare
8c64ffb
to
af9a0b7
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.
This pr looks good to go to me 🎉
I noticed that we still perform a lot of database queries in the linkcheck view, but since the performance is acceptable right now I think we can keep it that way for now. I pushed a commit to this branch that got rid of the lowest hanging fruits though 😃
Not working on integreat anymore
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.
Did you accidentally remove model changes during squashing? Did I miss something? I get `'Url' object has no attribute 'region_links' when I try to change links
5345776
to
e2524ae
Compare
@PeterNerlich Thank you for the review 😸 Yes, it seems I messed up while rebasing and/or squashing 😅 It should work again |
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.
It appears to work now, though on closer inspection rewriting links doesn't always yield the expected results (e.g. rewriting the willkommen link with its 3 usages, replacing it to link to …/wullkommen
and expecting it to appear in Invalid, also with 3 usages – instead it disappeared on me altogether) but this might just be a problem with the linkcheck library and not this PR.
Also, FYI, I saw a few errors like django.db.utils.IntegrityError: insert or update on table "linkcheck_link" violates foreign key constraint "linkcheck_link_url_id_c5f791e9_fk"
followed by DETAIL: Key (url_id)=(34) is not present in table "linkcheck_url".
on the console, but they as well are from within the linkcheck code and probably not caused by anything in this PR (rather, I suspect my local db state was at fault).
I hope this is the case, so I'll approve. The other comments are ideas non-essential to this PR as well.
f7fdfde
to
b1342ee
Compare
7753833
to
18c1132
Compare
Co-authored-by: David Venhoff <[email protected]> Co-authored-by: Peter Nerlich <[email protected]>
ff0cd8a
to
bb28096
Compare
Short description
This is an updated version of #2898 with improvement sugeestion by @david-venhoff (Thank you 🙏)
Proposed changes
replace_links
if users replace an URL per ✏️ in a row of the centalized link list (because we are not prefetching)*1Side effects
Resolved issues
Fixes: #1443
Pull Request Review Guidelines