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

[SDPAP-8290]added hook update to apply new content type to existing sites #122

Merged

Conversation

yeniatencio
Copy link
Contributor

@yeniatencio yeniatencio commented Dec 4, 2023

JIRA issue:
https://digital-vic.atlassian.net/browse/SDPAP-8290

Changed

  1. Created a hook update in tide_api to apply the changes to the existent sites.
  2. Reordered checkboxes as AC requested.

Note:
Added new and publication content type to content collection in baywatch dpc-sdp/baywatch#86
Testing link: https://nginx-php.pr-365.content-reference-sdp-vic-gov-au.sdp4.sdp.vic.gov.au/

Screenshots

Screenshot 2023-12-05 at 3 11 51 pm

Copy link
Contributor

@krakerag krakerag left a comment

Choose a reason for hiding this comment

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

One query @yeniatencio


if (!empty($temp)) {
$content_types_options = $temp + $content_types_options;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be a pain and ask why this code is here... it doesn't make a lot of sense to me just by looking at it, is it reordering the types for the checkboxes or something?

Copy link
Contributor Author

@yeniatencio yeniatencio Dec 6, 2023

Choose a reason for hiding this comment

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

@krakerag , part of the acceptance criteria is to have the checkboxes in this order:

Order of checkboxes:

Landing page

Publication (new)

News

Event (new)

That's why I have to reorder them as by default they are ordered alphabetically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a complicated piece of code to assign them when we know the order, we should just set the order and move on, rather than doing a temp assignment to an array and add it to another array etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems a complicated piece of code to assign them when we know the order, we should just set the order and move on, rather than doing a temp assignment to an array and add it to another array etc.

@krakerag , you mean, leave them by default? Alphabetically.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I mean literally just set them in the right order so the code is clear

['landing_page', 'publication', 'news', 'event']

if ($config) {
$config->set('content.field_content_collection_config.settings.content.internal.contentTypes.allowed_values', $allowed_content_types);
$config->save();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

@yeniatencio yeniatencio merged commit fed8ca9 into develop Feb 19, 2024
4 of 6 checks passed
@yeniatencio yeniatencio deleted the feature/SDPAP-8290-add-contenttype-content-collection branch February 19, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants