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

Clean up after the homepage builder work #6676

Merged
merged 25 commits into from
Jan 2, 2024

Conversation

sebastienhoorens
Copy link
Contributor

@sebastienhoorens sebastienhoorens commented Dec 14, 2023

This is a cleanup PR which we'll release in January 2024 (to have some time to be able to revert to the old homepage in case a customer is very unhappy with their new homepage). Review is therefore not urgent.

  • Homepages are no longer needed and are completely removed.
  • Removed the old craftjs_jsonmultiloc attribute from the layouts.
  • We noticed that pins are not used anywhere and removed those as well.
  • The homepage_builder flag is removed, as we completely switch to the new homepages.
  • The currently_working_on_text is no longer used.
  • Some last small fixes, like the sanitization of text in craftjs_json.

@cl-dev-bot
Copy link
Collaborator

cl-dev-bot commented Dec 14, 2023

Warnings
⚠️

The changelog is empty. What should I put in the changelog?

⚠️ The PR title contains no Jira issue key (case-sensitive)
⚠️ The branch name contains no Jira issue key (case-sensitive)
Messages
📖

Run the e2e tests

📖 Check translation progress

Generated by 🚫 dangerJS against f48808b

@sebastienhoorens sebastienhoorens changed the title Delete homepages and craftjs_jsonmultiloc Clean up after the homepage builder work Dec 15, 2023
Copy link
Contributor

@jamesspeake jamesspeake left a comment

Choose a reason for hiding this comment

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

All looks well tidied to me. Just a couple of comments

Comment on lines +9 to +11
UPDATE content_builder_layouts
SET content_buildable_id = NULL, content_buildable_type = NULL
WHERE content_buildable_type = 'HomePage';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely clear what's happening here, but given it is setting columns to null, just want to check there is no need for that data any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because the homepage was deleted, so the layouts linked to homepages can no longer reference them.

Comment on lines 37 to +38
when 'HomePage'
HomePage.first
nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better as an if/else or ternary as surely any other parameter of content_buildable will return nil too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm considering it tech debt, but the report builders should also be supported through this endpoint. I don't have a very strong preference, but I think it can be good to explicitly list all the expected content_buildable types (for example, here you can see that the report builders are (probably) not yet supported because they are not a part of the case statement).

@sebastienhoorens sebastienhoorens merged commit efbeff5 into master Jan 2, 2024
1 check passed
@sebastienhoorens sebastienhoorens deleted the cleanup-craftjs-and-homepage-settings branch January 2, 2024 11:05
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.

3 participants