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

feat(favorites): frontend revisions to pray and pay project #4580

Merged
merged 95 commits into from
Dec 19, 2024

Conversation

v-anne
Copy link
Contributor

@v-anne v-anne commented Oct 16, 2024

This is the PR for making frontend changes to the pray and pay user interface.

Mike had some great comments in #4507, so we'll start with them and see what's needed once the low-hanging fruit is done.

Additions include:

  • Allowing users to request a document from the recap_document.html page.
  • Revamping UI of top_prayers.html to provide better UX.
  • Creating user_prayers.html page to allow users to see their open and fulfilled prayers, and also summary statistics about how many requests they've had fulfilled. In a future PR, this will be made similar to tags to allow people (e.g., freelance journalists) to share documents they've requested.
  • documentation for how pray and pay project works.
  • minor grammar changes to email notifications.

@v-anne
Copy link
Contributor Author

v-anne commented Oct 16, 2024

@ERosendo this is the PR I'm going to build off of.

@mlissner
Copy link
Member

mlissner commented Oct 16, 2024

Seeing some commit messages around the user-prayer page. Reminds me that it doesn't have to be available in the profile section, just like tags aren't. The user prayers page could just be linked from the mouseover for each emoji or from the most-wanted page, for example.

@v-anne
Copy link
Contributor Author

v-anne commented Oct 16, 2024

Yeah, tags are what I am aiming to emulate.

@v-anne
Copy link
Contributor Author

v-anne commented Oct 16, 2024

image I'm having some issues getting the three buttons to display inline with each other in `recap_document.html`. Ideally, the prayer button would be to the right of the other two.

@v-anne
Copy link
Contributor Author

v-anne commented Oct 16, 2024

image

Having same issue with the button on the modal. There's also a related issue where if I click the prayer button repeatedly, the count will increase despite having exhausted the quota.

I gather this is an issue with the CSS, but I tried making some changes and was unsuccessful.

@mlissner
Copy link
Member

@ERosendo, do you think you could help, please?

@ERosendo
Copy link
Contributor

@mlissner sure! 👍

@v-anne
Copy link
Contributor Author

v-anne commented Oct 17, 2024

@ERosendo in addition to those two issues with the buttons, I've mostly finished a MVP of the user_prayers.html page.

This is what it currently looks like. The status column is broken; I can't figure out why the backend isn't passing the information through to the template. I intend to add a button to allow users to delete requests and one for them to download available documents as well.

image

@ERosendo ERosendo force-pushed the 4507-pray-and-pay-API branch from 4318237 to 141f78a Compare December 18, 2024 03:19
@ERosendo ERosendo force-pushed the 4507-pray-and-pay-API branch from 3e182c3 to 97fba33 Compare December 18, 2024 19:17
@ERosendo
Copy link
Contributor

@albertisfu Thanks for your feedback. I've implemented all your suggestions.

@ERosendo ERosendo requested a review from albertisfu December 18, 2024 19:19
@ERosendo ERosendo assigned albertisfu and unassigned ERosendo Dec 18, 2024
Copy link
Contributor

@albertisfu albertisfu left a comment

Choose a reason for hiding this comment

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

Thanks @ERosendo this looks great.

Just left a couple of additional comments and suggestions.

@@ -44,7 +45,7 @@ <h3 class="alt bottom" style="font-size: 1.5em; font-weight: normal; line-height
<td style="text-align: center">{{ docket_entry.entry_number }}{% if rd.attachment_number %}-{{ rd.attachment_number }}{% endif %}</td>
<td>
{% if docket_entry.datetime_filed %}
<span title="{{ docket_entry.datetime_filed|timezone:timezone}}">{{ docket_entry.datetime_filed|timezone:timezone|date:"M j, Y" }}</span>
<span title="{{ docket_entry.datetime_filed|datetime_in_utc}}">{{ docket_entry.datetime_filed|datetime_in_utc|date:"M j, Y" }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

In this line: {{ docket_entry.datetime_filed|datetime_in_utc|date:"M j, Y" }}, an empty value is displayed.

I believe the issue is that the datetime_in_utc filter handles the date format internally, so applying an additional format causes it to fail. If we want to display a different format here, we might need to use a different approach, such as modifying datetime_in_utc to accept an optional custom format.

Same for the .txt version

Copy link
Contributor

@ERosendo ERosendo Dec 18, 2024

Choose a reason for hiding this comment

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

we can pass timezone as a template variable instead of using the datetime_in_utc filter and use this custom format, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that's also a valid approach!

Comment on lines 269 to 285
filtered_list.values("recap_document")
.distinct()
.annotate(
price=Case(
When(recap_document__is_free_on_pacer=True, then=Value(0.0)),
When(
recap_document__page_count__gt=0,
then=Least(
Value(3.0),
F("recap_document__page_count") * Value(0.10),
),
),
default=Value(0.0),
)
)
.aaggregate(Sum("price", default=0.0))
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can avoid the duplicate code in this method and in get_lifetime_prayer_stats by encapsulating the shared logic into a separate method, something like:

async def compute_cost(queryset):
    return await (
        queryset.values("recap_document")
        .distinct()
        .annotate(
            price=Case(
                When(recap_document__is_free_on_pacer=True, then=Value(0.0)),
                When(
                    recap_document__page_count__gt=0,
                    then=Least(
                        Value(3.0),
                        F("recap_document__page_count") * Value(0.10),
                    ),
                ),
                default=Value(0.0),
            )
        )
        .aaggregate(Sum("price", default=0.0))
    )

And then just using:
cost = await compute_cost(filtered_list)

OR:

total_cost = await compute_cost(prayer_by_status.select_related("recap_document"))

Copy link
Contributor

Choose a reason for hiding this comment

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

You're correct! I'll create this new helper function.

Comment on lines 346 to 348
"count": prayer_count,
"num_distinct_purchases": distinct_prayers,
"total_cost": f"{total_cost:,.2f}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the same key names here as in PrayerStats?

I believe if we do something like:

Suggested change
"count": prayer_count,
"num_distinct_purchases": distinct_prayers,
"total_cost": f"{total_cost:,.2f}",
"prayer_count": prayer_count,
"distinct_count": distinct_prayers,
"total_cost": f"{total_cost:,.2f}",

Then in both returns we can do:
return PrayerStats(**data)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're correct! Thanks for bringing this to my attention.

@albertisfu albertisfu assigned ERosendo and unassigned albertisfu Dec 18, 2024
@ERosendo ERosendo requested a review from albertisfu December 18, 2024 23:39
@ERosendo ERosendo assigned albertisfu and unassigned ERosendo Dec 18, 2024
@ERosendo
Copy link
Contributor

@albertisfu Ready for another review 😃

@ERosendo ERosendo force-pushed the 4507-pray-and-pay-API branch from 74215f3 to 128de7c Compare December 19, 2024 00:09
Copy link
Contributor

@albertisfu albertisfu left a comment

Choose a reason for hiding this comment

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

Thanks! this now looks good to me :shipit:

@mlissner mlissner merged commit 5c54b88 into freelawproject:main Dec 19, 2024
10 checks passed
@mlissner
Copy link
Member

Right on. Into the maw it goes!! Thank you all for the teamwork polishing and refining this one. I'm looking forward to giving it a try.

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

Successfully merging this pull request may close these issues.

4 participants