Skip to content

fix(dashboards): modify myDashboards sorting logic #82364

Merged
harshithadurai merged 14 commits into
masterfrom
harshi/fix/dashboards-owner-sorting
Dec 20, 2024
Merged

fix(dashboards): modify myDashboards sorting logic #82364
harshithadurai merged 14 commits into
masterfrom
harshi/fix/dashboards-owner-sorting

Conversation

@harshithadurai

@harshithadurai harshithadurai commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

Modify myDashboards sorting logic. Previously, the myDashboards sort displayed the user's dashboards first followed by other dashboards sorted by creator id. This PR changes that so that the other dashboards are sorted by the creator's name.

Screenshot 2024-12-19 at 9 58 56 AM

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 19, 2024
@harshithadurai harshithadurai marked this pull request as ready for review December 19, 2024 14:47
@harshithadurai harshithadurai requested a review from a team December 19, 2024 14:47
@@ -163,12 +165,31 @@ def get(self, request: Request, organization) -> Response:
order_by = ["last_visited" if desc else "-last_visited"]

elif sort_by == "mydashboards":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we feature flag this so it doesn't rollout until the table also rolls out? So current users aren't affected and it'll make more sense with the table.

Also, should this get a different sort key? Instead of mydashboards it should be something like creator now, since it doesn't prioritize the request user anymore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense to be under myDashboards since it's still prioritizing the request user's dashboards and only changing the order of the remaining dashboards

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH oops, yeah, you're right. I misread the annotation as the sort, sounds good.

@narsaynorath narsaynorath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misread the diff the first time. Looks good now with the feature flags, just one comment about if the test needs to define the feature flag in the request too

@@ -163,12 +165,31 @@ def get(self, request: Request, organization) -> Response:
order_by = ["last_visited" if desc else "-last_visited"]

elif sort_by == "mydashboards":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH oops, yeah, you're right. I misread the annotation as the sort, sounds good.

Dashboard.objects.create(title="F", created_by_id=user_1.id, organization=self.organization)

self.login_as(user_1)
response = self.client.get(self.url, data={"sort": "mydashboards"})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to get wrapped with the with features.. context so it has the table feature flag?

@codecov

codecov Bot commented Dec 19, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #82364       +/-   ##
===========================================
+ Coverage   59.10%   80.44%   +21.34%     
===========================================
  Files        7296     7304        +8     
  Lines      321719   321956      +237     
  Branches    21017    20966       -51     
===========================================
+ Hits       190140   258991    +68851     
+ Misses     131173    62560    -68613     
+ Partials      406      405        -1     

@harshithadurai harshithadurai merged commit 5ef4ce3 into master Dec 20, 2024
@harshithadurai harshithadurai deleted the harshi/fix/dashboards-owner-sorting branch December 20, 2024 16:25
andrewshie-sentry pushed a commit that referenced this pull request Jan 2, 2025
Modify `myDashboards` sorting logic. Previously, the `myDashboards` sort
displayed the user's dashboards first followed by other dashboards
sorted by `creator id`. This PR changes that so that the other
dashboards are sorted by the creator's `name`.

<img width="150" alt="Screenshot 2024-12-19 at 9 58 56 AM"
src="https://github.com/user-attachments/assets/2ca9f7e9-db96-4837-aae9-bab3f735bfa5"
/>
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants