Skip to content
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

Superuser page for all opportunities. #450

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hemant10yadav
Copy link
Contributor

@hemant10yadav hemant10yadav commented Dec 9, 2024

@hemant10yadav hemant10yadav marked this pull request as draft December 9, 2024 06:07
@@ -16,21 +16,67 @@
</li>
{% endif %}

{% for page_number in page_obj.paginator.page_range %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to make this change because the previous pagination template displayed all the page numbers, and as the number of pages increased, it started overflowing.

@hemant10yadav hemant10yadav self-assigned this Dec 9, 2024
@hemant10yadav hemant10yadav marked this pull request as ready for review December 9, 2024 11:24
@hemant10yadav hemant10yadav changed the title Super user page for all opportunities. Superuser page for all opportunities. Dec 9, 2024
href="{% url 'opportunity:detail' org_slug=request.org.slug pk=opportunity.id %}">
<span class="bi bi-eye"></span><span class="d-none d-md-inline">&nbsp;View</span>
</a>
{% if request.org_membership.is_viewer %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The "Edit" and "Add Budget" buttons can be removed from this view.

@@ -1222,3 +1228,50 @@ def invoice_approve(request, org_slug, pk):
)
payment.save()
return HttpResponse(headers={"HX-Trigger": "newInvoice"})


class AllOpportunitiesView(SuperUserMixin, ListView):
Copy link
Contributor

Choose a reason for hiding this comment

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

We can build the list using the django-tables and django-filter libraries. It would make it easier to create a Table of Opportunities with filters.

Copy link
Member

@sravfeyn sravfeyn Jan 16, 2025

Choose a reason for hiding this comment

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

Yeah, this could be written with much less code (most of the HTML for filters and the table will be autogenerated, so you don't have to write it) if you go down that route. I have just made a PR that makes it more easier which you can base on.

Take a look at DeliveryStatsReport which is implemented in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure if we can create the exact same view using django-tables, as we have used custom sortable links in opportunity list page. However, the ticket clearly mentions that it should look the same as the normal Opportunity page. I’m open to discussing this further.

Copy link
Contributor

Choose a reason for hiding this comment

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

Django Tables has Meta options to enable sorting and ordering by columns. https://django-tables2.readthedocs.io/en/latest/pages/api-reference.html#Table.Meta

@@ -122,6 +123,11 @@ def test_func(self):
) or self.request.user.is_superuser


class SuperUserMixin(LoginRequiredMixin, UserPassesTestMixin):
Copy link
Member

Choose a reason for hiding this comment

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

There is already SuperUserRequiredMixin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I will remove it and use the existing SuperUserRequiredMixin.

@sravfeyn
Copy link
Member

Is it not possible to use the existing view OpportunityList to implement this for both superuser and org-specific users?

@hemant10yadav
Copy link
Contributor Author

Is it not possible to use the existing view OpportunityList to implement this for both superuser and org-specific users?

I am happy to use the same page, but doing so will make it much more complex, right? It is already being used for two cases: Opportunity and Managed Opportunity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants