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

1346 create recap pray and pay #4386

Merged
merged 55 commits into from
Sep 23, 2024

Conversation

v-anne
Copy link
Contributor

@v-anne v-anne commented Aug 30, 2024

This pull request implements the backend for #1346, the RECAP pray and pay project. It incorporates a few functions that (1) enforce the quota for users for maximum document requests each day, (2) that add document requests to the Prayer model, and (3) then update document requests after they are satisfied.

This is a work in progress and is just part of the MVP. Other features such as email notifications will come later.

@v-anne
Copy link
Contributor Author

v-anne commented Aug 30, 2024

At least two issues right now before this is ready for review.

  1. Not 100% sure which file in the settings directory to define the environment variable ALLOWED_PRAYER_COUNT.
  2. Not sure how to write the tests yet when this PR is only for the backend and the frontend isn't ready yet.

Would appreciate @mlissner's thoughts.

@mlissner
Copy link
Member

The var can go in misc.py or in project/prayers.py. I think the former is better until we have three or more settings for it.

There are whole books on your second question, but I think I'd fakers to make a user and some prayers, and then I'd make some little tests to make sure my functions are working properly. This barely needs tests, I agree, but you could check that the ranking is right and that people properly run out of prayers. You could also use time machine to play with the 24 hour timer?

@v-anne
Copy link
Contributor Author

v-anne commented Aug 30, 2024

There are whole books on your second question, but I think I'd fakers to make a user and some prayers, and then I'd make some little tests to make sure my functions are working properly. This barely needs tests, I agree, but you could check that the ranking is right and that people properly run out of prayers. You could also use time machine to play with the 24 hour timer?

Does this require me to be able to run the Docker container and everything else locally? I had issues with it the last time I tried so it might take me some time to manage it.

@mlissner
Copy link
Member

You could rely on Github actions to run the tests, if you want. Might be slow to iterate, but I suppose it'd work.

@v-anne
Copy link
Contributor Author

v-anne commented Sep 3, 2024

@mlissner I'm attempting to create some dummy data with Faker. I'm running into some issues, however. The code I just pushed was an attempt to create data for the Prayer model, which is dependent on there being data in the RECAPDocument model. I ran docker exec -it cl-django python /opt/courtlistener/manage.py make_dev_data, like the guide suggests doing, but neither model has any data in it after checking via the Django shell. Any thoughts? Other models like User do have dummy data after running this command, so any issue is likely limited to those two models.

EDIT: disregard this for now, I think I figured out my issue.

@v-anne
Copy link
Contributor Author

v-anne commented Sep 4, 2024

I'm going to confess error, @mlissner. Not sure how to solve the issue above.

@mlissner
Copy link
Member

mlissner commented Sep 6, 2024

@v-anne, sorry to leave you hanging. @albertisfu, do you think you could find a minute to help Vijay get their tests working?

@mlissner
Copy link
Member

mlissner commented Sep 6, 2024

(Just FYI, Vijay, Alberto has a few other things in his queue before this one, sorry!)

@v-anne
Copy link
Contributor Author

v-anne commented Sep 7, 2024

No worries, Mike. In the interest of time, I will try to work on the front end so it can be included in this PR too.

@v-anne
Copy link
Contributor Author

v-anne commented Sep 11, 2024

Just pushed a rough front end, I'm going to hold off on making any substantial changes to any of this until @albertisfu has a chance to weigh in. I think this PR is 85% of the way there, if Alberto can identify what the issues are with the fakers for the Prayer and RECAPDocument models, I'll be able to fully test and then have a finalized PR.

@v-anne
Copy link
Contributor Author

v-anne commented Sep 14, 2024

Started discussion #4459

@albertisfu
Copy link
Contributor

@mlissner @v-anne I've applied the following changes:

  • Moved the prayer methods to cl.favorites.utils and made them async.
  • Added tests for the prayer methods.
  • Added an integration test that changes the status of related Prayer instances when a document is granted.
  • Fixed the create_prayer method so that all tests pass.
  • Made the following adjustments to the get_top_prayers query:
    • Only checks for RECAPDocuments that have a related Prayer, as it was previously checking every RECAPDocument in the database.
    • Returns only the fields required to display on the leaderboard and create the PACER document URL.
  • Improved the leaderboard template:

The HTML now looks like this:
Screenshot 2024-09-20 at 5 08 28 p m

I'm wondering which fields we need to display here and if a link to PACER is required?

I have a question:

Is it possible for a user to pray for the same RECAPDocument multiple times? Or should there be only one prayer per user per document? If it's the latter, we might need to add a unique constraint to the model and tweak the prayer_eligible method.

Is there anything else I can help with?

Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

This looks good to me with a few little things.

Thanks Alberto for your work and review! Your comments all sounds great.

Is it possible for a user to pray for the same RECAPDocument multiple times? Or should there be only one prayer per user per document? If it's the latter, we might need to add a unique constraint to the model and tweak the prayer_eligible method.

Good point. Yes, we'll want to do that.

I'm wondering which fields we need to display here and if a link to PACER is required?

We've got a lot of work to do on that, but I think we can launch it as an MVP now, right?

cl/favorites/views.py Show resolved Hide resolved
cl/favorites/urls.py Outdated Show resolved Hide resolved
@v-anne
Copy link
Contributor Author

v-anne commented Sep 21, 2024

Alberto, thank you so much for your work on this. My responses are below.

I'm wondering which fields we need to display here and if a link to PACER is required?

I think this is alright for this PR. It will need some changes but this PR currently doesn't allow users to actually add prayers from the front end yet, so it's ok if it's not perfect.

I have a question:

Is it possible for a user to pray for the same RECAPDocument multiple times? Or should there be only one prayer per user per document? If it's the latter, we might need to add a unique constraint to the model and tweak the prayer_eligible method.

I agree with Mike. It was not my attention to allow that. Accordingly, I added a constraint to the Prayer model and adjusted the create_prayer method to prevent one user requesting the same document multiple times.

Is there anything else I can help with?

I think the last thing needed before this PR can be merged is to figure out why the test suite is failing after the migration is applied.

Again, thank you for revising the issues with my code.

@mlissner
Copy link
Member

Looking good! It looks like you're missing the migration file and that's why tests are failing.

@v-anne
Copy link
Contributor Author

v-anne commented Sep 21, 2024

I think it's good to go now.

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.

@v-anne I reviewed your changes. They look good. I just made a small tweak to the create_prayer method to simplify it and added a related test.

I also added the SQL migration file for the unique constraint migration.

Finally, I have a question about whether the open_prayers view should be private or not.

Thanks!

cl/favorites/views.py Outdated Show resolved Hide resolved
@albertisfu
Copy link
Contributor

@mlissner If there are no more comments from your side, I think this could be merged. It will require applying the migration on prod.

@mlissner mlissner merged commit 21d74fb into freelawproject:main Sep 23, 2024
9 checks passed
@mlissner
Copy link
Member

Merged! Thank you both! I'll get it deployed in a moment, over here: https://github.com/freelawproject/courtlistener/actions/runs/11002188561

@mlissner
Copy link
Member

We're in business! https://www.courtlistener.com/prayers/top/

@mlissner
Copy link
Member

I mean, sort of!

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

Successfully merging this pull request may close these issues.

3 participants