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

Email application url #1491

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

Email application url #1491

wants to merge 4 commits into from

Conversation

ranta
Copy link
Collaborator

@ranta ranta commented Jan 15, 2025

🛠️ Changelog

  • Add Application and Application Section IDs to relevant email links
  • Add better coverage for email tests
  • Fix incorrect variable accessing in some staff emails

🧪 Test plan

  • Automated tests

🚧 Dependencies

  • None

🎫 Tickets

@ranta ranta requested review from vergama and matti-lamppu January 15, 2025 16:19
Comment on lines +42 to +46
# Applied to all tests
pytestmark = [
pytest.mark.django_db,
]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't add this on the top level in this case, since most of the tests in this file don't need database access. It just makes them run slower.

@@ -81,7 +81,7 @@ def actions(self) -> LocationActions:

@property
def address(self) -> str:
return f"{self.address_street}, {self.address_zip} {self.address_city}"
return ", ".join([self.address_street, f"{self.address_zip} {self.address_city}".strip()])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this some lint rule? I think the previous version was more readable.

Comment on lines +232 to +253
def get_my_applications_ext_link(
*,
language: Lang,
application_section: ApplicationSection | None = None,
) -> str:
"""
Return the link to the 'My applications' page:
e.g. https://varaamo.hel.fi/applications/{application_id}/view?tab=reservations&section={application_section_id}
"""
url = settings.EMAIL_VARAAMO_EXT_LINK.removesuffix("/")

if language != "fi":
return f"{url_base}/{language}/applications"
return f"{url_base}/applications"
url = f"{url}/{language}"
url = f"{url}/applications"

if application_section:
application_id = getattr(application_section, "application_id", None)
application_section_id = getattr(application_section, "id", None)

url = f"{url}/{application_id}/view?tab=reservations&section={application_section_id}"

return url
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would create a separate function to create the section link. Then I would give that function more specific info instead of the whole ApplicationSection, e.g. just the application_id and application_section_id, which would allow us to generate the same link using test data.

}
else:
data.update(get_contex_for_seasonal_reservation_check_details_url(language=language))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this doesn't get the section, it creates a different link when using e.g. the email tester in django admin. This might be confusing, so I would make this a different function that can create the same link even without the section itself.

Copy link
Collaborator

@matti-lamppu matti-lamppu left a comment

Choose a reason for hiding this comment

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

Some comments, but nothing major. Take it or leave it.

@matti-lamppu matti-lamppu added the improvement Improves an existing feature label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants