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

Do not archive mirrored pages #2329

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

MizukiTemma
Copy link
Member

Short description

This PR solves the inconsistent state of mirrored pages by prevanting archiving of mirrored pages.

Proposed changes

  • Deactivate the archiving icon of mirrored pages in the tree
  • Hide the archiving button in the form if the page is mirrored
  • Check if there are mirorring pages before archiving a page (both in single request and bulk action)
  • Adjust error/success messages of the bulk action and titles of the icon

Side effects

  • None?
  • I have chosen the first one among the two suggested solutions (expected behaviour) because it seems to be more in accordance with the existing behaviour of page deletion and probably causes less confusion than the second option.

Resolved issues

Fixes: #2171


Pull Request Review Guidelines

@MizukiTemma MizukiTemma requested a review from a team as a code owner July 1, 2023 16:21
@codeclimate
Copy link

codeclimate bot commented Jul 1, 2023

Code Climate has analyzed commit 076d180 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 76.3%.

View more on Code Climate.

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot, works as expected! 💪

I thought that maybe it could be helpful to know which pages prevent the page from being archived, so in case they want to proceed they can e.g. contact the manager of the other region to remove the live content etc...

@MizukiTemma
Copy link
Member Author

MizukiTemma commented Jul 3, 2023

@timoludwig In the action box it shows where the page is mirrored. I'll modify the explanation a bit.

Where am I mirrored

Update: now the warning with a list of mirroring pages is shown both for archiving and deletion. If a page is only mirrored and does not have any child pages (another reason deletion will be deactivated), the warning is shown doubled, though I hope this is not so tragic and rather helps user as explizit explanation.

@MizukiTemma MizukiTemma force-pushed the bug/do_not_archive_mirrored_pages branch from 6c54b1f to d0a975b Compare July 3, 2023 12:09
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Cool, thanks! 👍

@svenseeberg
Copy link
Member

I'm a little worried that some regions have to maintain pages because other regions do mirror them. Can we think of a solution here? Maybe a button to remove all mirror pages by force from the source region and notify the maintainers via mail?

@MizukiTemma
Copy link
Member Author

MizukiTemma commented Jul 4, 2023

That's a nice idea 😃 @osmers What way do you think is the best to inform content creators that a mirrored page has been removed if we introduce force archiving/deleting of mirrored pages? I have a bit of worry that not all of them check E-mails...

Though I would make it a separate issur.

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.

Looks good to me. As a small suggestion, you might consider to move the is mirrored checks into the page.archive() method, to reduce code duplication. I don't think this is blocking the pr though.

integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
@MizukiTemma
Copy link
Member Author

As a small suggestion, you might consider to move the is mirrored checks into the page.archive() method, to reduce code duplication. I don't think this is blocking the pr though.

Nice idea, though I'll leave the code as it is to keep the current check & message schema and to avoid bulk archive action getting inturruped at the first mirrored page 🙂

Co-authored-by: David Venhoff <venhoff@integreat-app.de>
@MizukiTemma MizukiTemma force-pushed the bug/do_not_archive_mirrored_pages branch from ff1120d to 076d180 Compare July 5, 2023 23:10
@MizukiTemma MizukiTemma merged commit 5df9053 into develop Jul 5, 2023
5 checks passed
@MizukiTemma MizukiTemma deleted the bug/do_not_archive_mirrored_pages branch July 5, 2023 23:36
@osmers
Copy link

osmers commented Jul 24, 2023

@MizukiTemma good question - I think generally they are registered with their work account and should therefore read the emails. Maybe you can also always send an email to me so that I am aware and can inform the corresponding person from the service team to double check with the municipality.
I just had the issue as well that I need to archive the corona pages but can't bcs they are mirrored in many regions.
Did you introduce a solution already or not yet?

@MizukiTemma
Copy link
Member Author

@osmers Thank you for feedback! This notification function is not yet implemented. I have updated the issue for that #2338

@osmers
Copy link

osmers commented Jul 25, 2023

@MizukiTemma at the same time I noticed that archived pages are also shown as mirroring a page and created an issue for that bcs only live pages should be in the list. Otherwise people would get notified that content has been removed even though they already archived the page that had the removed content.

@MizukiTemma
Copy link
Member Author

@osmers Thank you for report and creating the issue!

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.

Archiving mirrored pages causes inconsistent state
5 participants