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(templates): Enhance I'm Now Following [#332] #530

Merged
merged 14 commits into from
May 8, 2024

Conversation

TheCleric
Copy link
Contributor

@TheCleric TheCleric commented May 1, 2024

This should implement #332

Things I definitely want others' input on:

  • The code for is_bankruptcy, which I went with what SEEMED correct, but I could be very wrong (see comment in that function).
  • The new templating code that now supports Django templates (see notes below).
  • A little bit more strict typing in the CL api code (just to give me some extra insurance that my code was correct).
  • I didn't add any new tests to the test_tasks.py file. Not sure if that would be good as well, or would overlap a ton with the tests in test_templates.py that I added.

For the template code, as I was adding the desired feature, I experimented with a few different ways of doing it. I first attempted to continue our current path of 1 template per option. Since before we only had one option, that was fine since that only meant two templates. However I discovered this grows exponentially. 2 options means 4 templates (e.g., post, post w/ article, post w/ date, post w/ article + date). And 3 options means 8 templates.

This was starting to get unwieldy (especially for the code for deciding which template to use). I also thought about adding something like sub templates where you could inject templates into other templates for some of this conditional stuff. This just felt like writing my own templating engine (poorly). My next experiment was trying to do a lazy loaded f string so I could use f string conditionals in the templates. This proved to be a bit unwieldy as well since to do so I would either have to make the template strings into lambda functions (which seemed funky) or you can populate an f string by passing it into eval (which sounds like a security nightmare waiting to happen).

So ultimately the only other choice I could think of boiled down to "use a template language" and since we're using Django, we essentially get one out of the box.

However I know that this may feel like overkill, and others may not share my opinions and evaluations of the other solutions, or even come up with a solution that I didn't think of. So this part is definitely flexible. I just wanted to get something working and in front of you and then iterate from there if needed.

@TheCleric TheCleric force-pushed the feat-332-now-following-date branch from 3bbb1c6 to 291e427 Compare May 1, 2024 18:51
@mlissner
Copy link
Member

mlissner commented May 1, 2024

That sounds great, @TheCleric. Let us know when this is ready for review and we'll dig in. Totally makes sense about the template question. The devil will be in the details, but I follow your logic!

@TheCleric
Copy link
Contributor Author

Okay I've finished testing and this seems good to go for me with one big caveat: I don't think I can do the Twitter API without paying money, so that would need someone else to validate.

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.

Just one comment for me. I leave the full review to you @ERosendo.

bc/subscription/tasks.py Outdated Show resolved Hide resolved
bc/subscription/tasks.py Outdated Show resolved Hide resolved
bc/subscription/tasks.py Outdated Show resolved Hide resolved
@TheCleric
Copy link
Contributor Author

@ERosendo thanks for the feedback. I think I've addressed what you pointed out.

@ERosendo
Copy link
Contributor

ERosendo commented May 8, 2024

The code looks good and the templates seem well-structured. @mlissner, if you're happy with the content of the templates, I believe we can merge this PR.

Here are screenshots showcasing the different templates:

New cases

  • Posts with context and initial complaint available for thumbnails

    image

    image

    image

  • Posts with no context and petition available for thumbnails

    image

    image

    image

  • Posts with context and no petition/complaint available for thumbnails

    image

    image

    image

Old cases

  • Posts with context and initial complaint available for thumbnails

    image

    image

    image

  • Posts with no context and petition available for thumbnails

    image

    image

    image

  • Posts with context and no petition/complaint available for thumbnails

    image

    image

    image

@mlissner mlissner merged commit 56e1647 into freelawproject:main May 8, 2024
5 checks passed
@mlissner
Copy link
Member

mlissner commented May 8, 2024

Looks awesome! This is a huge improvement, thank you for the review Eduardo, and thank you for the great work, @TheCleric. So cool!

@mlissner mlissner linked an issue May 8, 2024 that may be closed by this pull request
@TheCleric TheCleric deleted the feat-332-now-following-date branch May 8, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

More enhancements to "I'm now following" posts
3 participants