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

Include links of imprints into link replacement #2983

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

MizukiTemma
Copy link
Member

@MizukiTemma MizukiTemma commented Aug 9, 2024

Short description

This PR adds ImprintPageTranslation into the target models of replace_links so links in imprints can be replaced too by the "Search&Replace" on the uppr right corner of the broken link list.

Proposed changes

  • Add ImprintPageTranslation into the list models in the function replace_links

Side effects

  • Should we probaly think of centralised check for such process of adding/removing a model to/from our link features? Somthing like, listing models in one single list (as an environment variable, for exmaple) and we add a check whether all models of the list are included at the points where you need to add changes while removing/adding a model to the link check feature.

Resolved issues

Fixes: #2982


Pull Request Review Guidelines

Copy link

codeclimate bot commented Aug 9, 2024

Code Climate has analyzed commit 8e18bed and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 83.3% (0.0% change).

View more on Code Climate.

Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Thanks!

Should we probaly think of centralised check for such process of adding/removing a model to/from our link features? Somthing like, listing models in one single list (as an environment variable, for exmaple) and we add a check whether all models of the list are included at the points where you need to add changes while removing/adding a model to the link check feature.

I think this is a good idea. For this case though I think it would be best to just completely rewrite the replace_links function. Over time, it has become quite messy and now we have the get_region_links function that we could use instead to make replace_links both faster and cleaner.
Do you want to do that in this pr? Otherwise I could also do that in a follow-up pr.

integreat_cms/release_notes/current/unreleased/2982.yml Outdated Show resolved Hide resolved
@MizukiTemma
Copy link
Member Author

@david-venhoff Thank you for review and idea 😸 I'd suggest to do it in another PR to stay in the policy "one PR, one purpose".

@MizukiTemma MizukiTemma force-pushed the bug/include_imprint_links_in_link_replace branch from 1eeb11b to 24a8537 Compare August 12, 2024 07:28
@MizukiTemma MizukiTemma force-pushed the bug/include_imprint_links_in_link_replace branch from 24a8537 to 8e18bed Compare August 12, 2024 07:28
Copy link
Contributor

@deen13 deen13 left a comment

Choose a reason for hiding this comment

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

👍 😄

@MizukiTemma MizukiTemma merged commit 0725bf2 into develop Aug 12, 2024
5 checks passed
@MizukiTemma MizukiTemma deleted the bug/include_imprint_links_in_link_replace branch August 12, 2024 07:57
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.

replace_links does not replace links of Imprints
3 participants