-
Notifications
You must be signed in to change notification settings - Fork 989
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
profile: add archived projects section #17571
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
{% if projects %} | ||
{% if project_count %} |
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 dug through this, and realized what was bothering me.
After this change, we confirm that there's actual projects, but the way the HTML is structured we end up with two package-list
entries, and nothing differentiates them. Could we/should we combine the logic so that there's one instance of a package-list
or at least provide some context to the div class that these are active
vs archived
? Or is that overthinking it for classes that don't have any CSS specific to them right now? (Ignore for a second that the CSS for package-list
was removed a long time ago.)
This is not a blocker, but I wanted to note it in case someone thinks it's important for later.
warehouse/accounts/views.py
Outdated
|
||
return { | ||
"user": user, | ||
"projects": live_projects, |
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.
question: considering the only variable use in the template of projects
is now to combine to project_count and to iterate over it, any reason to not have consistency and rename it live_projects
to reduce cognitive load?
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.
Yeah, good call. I didn't rename it because I didn't want to churn a template variable but it's definitely confusing as-is!
|
||
return {"user": user, "projects": projects} | ||
if row.lifecycle_status == LifecycleStatus.Archived: | ||
archived_projects.append(project) |
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 conditional is not expressed by tests just yet. Considering this change is also mucking about in the template-land, maybe a functional test is in order.
Likely location: https://github.com/pypi/warehouse/blob/e8195e40986d63f9b72926a67eb6acfd43b36932/tests/functional/test_user_profile.py
I think you've done some client-side HTML assertions in other functional tests before, let me know if you need any pointers!
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.
Done! I've added both a functional test and a unit-level one: the functional test confirms that the view doesn't explode when archived projects are present, and the unit test confirms that the live_projects
/archived_projects
states are what we expect.
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Head branch was pushed to by a user without write access
This splits the project list in a user's public profile into two groups: "active" and "archived". Both are rendered as before, but archived projects are additionally given their own header (linked to a new help section) and each project is given a little "archived" badge that's identical to the badge elsewhere.
Example render:
CC @miketheman for feedback 🙂
Closes #17550.