-
Notifications
You must be signed in to change notification settings - Fork 116
feat(ui): Add warning signpost for old snaps #5352
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
base: main
Are you sure you want to change the base?
Conversation
|
gajeshbhat is not a collaborator of the repo |
6c05c8c to
5d57e46
Compare
|
Thanks again @gajeshbhat. I'll pass it through copilot first and we'll add it to our maintenance review list. With feature requests like this, we usually get them through UX first, before even starting implementation. While your suggestion is OK, it hasn't been decided where such notification should be added, what should it say, how old snap needs to be for it to show up. That said, we can use your PR to discuss these with UX during review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5352 +/- ##
==========================================
- Coverage 66.80% 0 -66.81%
==========================================
Files 113 0 -113
Lines 3714 0 -3714
Branches 965 0 -965
==========================================
- Hits 2481 0 -2481
+ Misses 1098 0 -1098
+ Partials 135 0 -135 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR adds a warning notification system for snap packages that haven't been updated in over 2 years, helping users identify potentially outdated or unmaintained snaps.
- Added backend logic to detect old snaps and format dates appropriately
- Implemented UI notification component in the snap details sidebar
- Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| webapp/store/snap_details_views.py | Integrates old snap detection into snap details context |
| webapp/store/logic.py | Implements core functions for detecting old snaps and date formatting |
| tests/store/tests_public_logic.py | Adds comprehensive test coverage for new date and old snap logic |
| templates/store/snap-details/_details.html | Adds warning notification UI component to snap details sidebar |
| static/sass/styles.scss | Adds custom styling for the old snap warning notification |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
webapp/store/logic.py
Outdated
| years_diff = days_diff / 365.25 # Account for leap years | ||
| years_since_update = int(years_diff) | ||
|
|
||
| is_old = days_diff >= (old_threshold_years * 365) |
Copilot
AI
Sep 5, 2025
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 calculation uses a fixed 365 days per year while line 248 correctly uses 365.25 to account for leap years. This inconsistency could cause edge cases where years_diff and is_old don't align. Use (old_threshold_years * 365.25) for consistency.
| is_old = days_diff >= (old_threshold_years * 365) | |
| is_old = days_diff >= (old_threshold_years * 365.25) |
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.
Updated! Changed to use (old_threshold_years * 365.25) for consistency with the leap year calculation on line 248. Updated tests accordingly to handle the more precise calculation.
static/sass/styles.scss
Outdated
| } | ||
|
|
||
| .old-snap-warning { | ||
| border: none !important; |
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.
Not sure what's the reasoning behind this. Ideally we would not want to modify existing Vanilla patterns like that. Especially the use of !important is a bad practice in CSS.
Notification does have a "borderless" variant if you were aiming at something like that: https://vanillaframework.io/docs/patterns/notification#appearance
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. I will look into the variant in the docs linked above. I was trying to style the box. My CSS knowledge is definitely below average so I will research and update.
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 for the link. Removed the custom CSS with !important and switched to using Vanilla Framework's borderless variant: p-notification--caution is-borderless
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.
Kindly note that the orange line on the left in the screenshot is gone, as is-borderless class removes all borders. I will update the screenshots. I think we can leave it like that as the caution symbol still conveys the message and looks cleaner. I will wait for comments from UX before I make any other modifications.
Thanks for taking a look @bartaz . Yes, I do remember seeing the |
5d57e46 to
07d256e
Compare
a434581 to
53a0829
Compare
53a0829 to
25f90a3
Compare
25f90a3 to
687ff8e
Compare
e07aeea to
ea05e2c
Compare
ea05e2c to
4a59aa1
Compare
27403bc to
d62b840
Compare
|
Hey @bartaz , Hope you have been well. Any chance we can get a review from UI/UX for this PR ? |
0e38842 to
cbc1ba9
Compare
da5889d to
84e8a3e
Compare
|
Retriggerd CI using |
- Add is_snap_old() function to detect snaps older than 2 years - Add convert_date_month_year() for cleaner date formatting - Display warning notification on snap details for old snaps - Use simple "Month Year" format for better readability - Add custom styling with orange left border for consistency - Include comprehensive test coverage for date logic - Message: "This snap in this channel hasn't been updated since [Month Year]. Contact the developer to ask for an update." Fixes canonical#2988
84e8a3e to
7953a7d
Compare
Done
Adds a warning notification on snap detail pages for snaps that haven't been updated in over 2 years, helping users identify potentially outdated or unmaintained snaps.
Fixes #2988
Changes
is_snap_old()function to detect snaps older than 2 yearsconvert_date_month_year()for cleaner "Month Year" formatImplementation Details
p-notification--cautionwith custom.old-snap-warningclassHow to QA
Unit Testing
is_snap_old()functionconvert_date_month_year()functionPrerequisites
feat/signpost-old-snaps-2988dotrunTest Cases
Snaps that SHOULD show the warning (2+ years old):
http://localhost:8004/mumble- Should show a warninghttp://localhost:8004/hello- Should show a warninghttp://localhost:8004/wekan- Should show a warningSnaps that should NOT show the warning (recently updated):
http://localhost:8004/vault- Should NOT show warninghttp://localhost:8004/microk8s- Should NOT show warningWhat to Look For
Warning Appearance:
Message Format:
Positioning:
Edge Cases to Test
Screenshots
Screenshots are attached below showing:
Warning displayed on old snaps (mumble, hello, wekan)
No warning on recently updated snaps (vault, microk8s)
Notes