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

Add US-only option in Collection #1748

Merged
merged 26 commits into from
Jan 30, 2025
Merged

Add US-only option in Collection #1748

merged 26 commits into from
Jan 30, 2025

Conversation

Divs-B
Copy link
Contributor

@Divs-B Divs-B commented Jan 16, 2025

Co-authored by @fredex42 (Thanks for help in Backend and DB wiring!)

What's changed?

We are now moving towards country specific curation of data,
We want to try for US first to provide much more in-depth use of Feast App for US users.
We are going to put US only option to Container(Collection) level to help Editorial users to select option if they want to mark it for only to display in US.

  1. UI section: The change will have dropdown menu now from headlessui/react library, this will only be for Feast Editions at the moment.
    (Note: As we might need to get other devs on Fronts tool and UX help to confirm button's look and feel so its style and appearance are subject to change.)
Screen.Recording.2025-01-30.at.11.50.27.mov

(updated video above)

  1. Backend and DB wiring section: Thanks @fredex42 for help in wiring this end to end (thanks to @jonathonherbert too for confirming on ways to achieve this in the code).
    This wiring will help in supplying the data to Database and to backend for our Apps team to use at the end.

  2. Wired the UI change with backend with new endpoint in the routes(/update-regions), which will PATCH the status for regions as per User's selection.

Checklist

General

  • 🤖 Relevant tests added
  • ✅ CI checks / tests run locally
  • 🔍 Checked on CODE

Client

  • 🚫 No obvious console errors on the client (i.e. React dev mode errors)
  • 🎛️ No regressions with existing user interactions (i.e. all existing buttons, inputs etc. work)
  • 📷 Screenshots / GIFs of relevant UI changes included

@Divs-B Divs-B requested a review from a team as a code owner January 16, 2025 09:49
@fredex42 fredex42 force-pushed the db/add-us-only-option branch from 97dabf1 to b302b74 Compare January 22, 2025 17:30
@Divs-B Divs-B force-pushed the db/add-us-only-option branch from 591d4fd to ec5aa88 Compare January 28, 2025 13:00
@Divs-B Divs-B requested a review from Georges-GNM January 28, 2025 13:03
@Divs-B
Copy link
Contributor Author

Divs-B commented Jan 28, 2025

We have tested this by deploying on CODE and publishing the Front.
The data it is sending is including regions fields.

@@ -62,6 +62,8 @@ describe('Store middleware', () => {
draft: [],
previously: undefined,
type: 'type',
targetedRegions: [], //add here to pass the test for persistCollectionOnEdit
Copy link
Contributor

@Georges-GNM Georges-GNM Jan 28, 2025

Choose a reason for hiding this comment

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

think we can get rid of the comment

Suggested change
targetedRegions: [], //add here to pass the test for persistCollectionOnEdit
targetedRegions: [],

Copy link
Contributor

Choose a reason for hiding this comment

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

similarly, can probably remove the comments

@@ -657,6 +671,8 @@ const stateWithCollection: any = {
draft: [],
previously: undefined,
type: 'type',
targetedRegions: [], //add here to pass the test
Copy link
Contributor

@Georges-GNM Georges-GNM Jan 28, 2025

Choose a reason for hiding this comment

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

nit: comment

Suggested change
targetedRegions: [], //add here to pass the test
targetedRegions: [],

@@ -519,6 +521,8 @@ const collection = {
id: 'exampleCollection',
displayName: 'Example Collection',
type: 'type',
targetedRegions: [], //mandatory, should be added here to compile
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comment

Suggested change
targetedRegions: [], //mandatory, should be added here to compile
targetedRegions: [],

Comment on lines 31 to 38
// const curatedPlatformStrategy = () =>
// renamingCollection
// ? renameEditionsCollection(id)(collectionToEditionCollection(collection))
// : isMarkedForUSOnly
// ? markCollectionForUSOnly(id)(collectionToEditionCollection(collection))
// : updateEditionsCollection(id)(
// collectionToEditionCollection(collection),
// );
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can delete this unused code

Suggested change
// const curatedPlatformStrategy = () =>
// renamingCollection
// ? renameEditionsCollection(id)(collectionToEditionCollection(collection))
// : isMarkedForUSOnly
// ? markCollectionForUSOnly(id)(collectionToEditionCollection(collection))
// : updateEditionsCollection(id)(
// collectionToEditionCollection(collection),
// );

@@ -1473,6 +1473,8 @@ export const finalStateWhenAddNewCollection = {
platform: undefined,
targetedTerritory: undefined,
type: undefined,
targetedRegions: [], //add here to pass the test for add collection
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
targetedRegions: [], //add here to pass the test for add collection
targetedRegions: [],

@@ -1491,6 +1493,8 @@ export const finalStateWhenAddNewCollection = {
platform: undefined,
targetedTerritory: undefined,
type: undefined,
targetedRegions: [], //add here to pass the test for add collection
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
targetedRegions: [], //add here to pass the test for add collection
targetedRegions: [],

@@ -2233,6 +2237,8 @@ export const finalStateWhenRemoveACollection = {
platform: undefined,
targetedTerritory: undefined,
type: undefined,
targetedRegions: [], //add here to pass the test for remove collection
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
targetedRegions: [], //add here to pass the test for remove collection
targetedRegions: [],

@@ -196,6 +198,8 @@ describe('Collection actions', () => {
live: ['abc', 'def'],
previously: undefined,
type: 'type',
targetedRegions: [], //add here to pass the test for Get Collections thunk
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
targetedRegions: [], //add here to pass the test for Get Collections thunk
targetedRegions: [],

@@ -130,6 +130,8 @@ describe('Collection actions', () => {
live: ['abc', 'def'],
previously: undefined,
type: 'type',
targetedRegions: [], //add here to pass the test
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
targetedRegions: [], //add here to pass the test
targetedRegions: [],

Copy link
Contributor

@fredex42 fredex42 left a comment

Choose a reason for hiding this comment

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

Couple of very small comments but otherwise looking good to me

def updateCollectionRegions(collectionId: String) = EditEditionsAuthAction(
parse.json[EditionsFrontendCollectionWrapper]
) { req =>
logger.info(s"Mark for US only the collection ${collectionId}")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is inaccurate - i think it can just be zapped now it's working tbh

)}
{isFeast && (
<>
{targetedRegions?.length > 0 ? 'US!' : ''}{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's replace this text with a US flag emoji: https://emojipedia.org/flag-united-states

Copy link
Contributor

Choose a reason for hiding this comment

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

bonus if you can get a tooltip on there to say "Targeted to US only" 😁

{isFeast && (
<>
{targetedRegions?.length > 0 ? 'US!' : ''}{' '}
{/*The above text will appear on collection to show US tagged collections, for testing purpose only.*/}
Copy link
Contributor

Choose a reason for hiding this comment

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

comment ⚡

@@ -302,6 +306,8 @@ async function buildCollection(collectionName, frontId, count) {
}

async function frontNameToId(issueId, frontName) {
console.log(`${frontsBaseUrl}/editions-api/issues/${issueId}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log(`${frontsBaseUrl}/editions-api/issues/${issueId}`);

Copy link
Contributor

Choose a reason for hiding this comment

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

@Divs-B sorry, i left this one in!

@Divs-B Divs-B force-pushed the db/add-us-only-option branch from 1c49330 to 325f8c7 Compare January 29, 2025 11:56
>
US Only
<Icon>
{targetedRegions.includes('us') ? <h2>&#x2713;</h2> : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the &#2713 isn't obvious to me, I think you can use

Suggested change
{targetedRegions.includes('us') ? <h2>&#x2713;</h2> : undefined}
{targetedRegions.includes('us') ? <h2>&check;</h2> : undefined}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Georges-GNM for the suggestion, I will take this to test its look and feel in later PR if its ok for you?
This PR is good to go as got approved by you and @fredex42

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, the icons are working as is, it's more for (my) code readability (as I had no idea what that thing was doing before looking it up 😅 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Georges-GNM

setMenuOpen((prev) => !prev);
}}
>
<pre style={{ padding: 0, margin: 0 }}>&#x22EE;</pre>
Copy link
Contributor

Choose a reason for hiding this comment

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

could use

Suggested change
<pre style={{ padding: 0, margin: 0 }}>&#x22EE;</pre>
<pre style={{ padding: 0, margin: 0 }}>&vellip;</pre>

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 one too, I will take this to test its look and feel in later PR if its ok for you?

Copy link
Contributor

@Georges-GNM Georges-GNM left a comment

Choose a reason for hiding this comment

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

A massive PR, but I appreciate somewhat difficult to separate too. Few nitpicky comments, tested on code and feast as well as regular collections still seem to behave as intended, so it looks good to me - if Andy's happy, then it's a 👍 from me.

As this feels like a big change (even if it should only be limited to feast), might be worth mentioning to CP once the PR's merged in case there's anything we've missed.

Copy link
Contributor

@fredex42 fredex42 left a comment

Choose a reason for hiding this comment

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

🚢

@Divs-B Divs-B merged commit 5d6c14b into main Jan 30, 2025
13 checks passed
@Divs-B Divs-B deleted the db/add-us-only-option branch January 30, 2025 12:03
@prout-bot
Copy link

Seen on PROD (merged by @Divs-B 7 minutes and 29 seconds ago) Please check your changes!

@@ -133,11 +139,10 @@ const CollectionMeta = styled(CollectionMetaBase)`
const ItemCountMeta = CollectionMetaBase;

const CollectionHeadingSticky = styled.div`
position: sticky;
Copy link
Contributor

@emdash-ie emdash-ie Jan 31, 2025

Choose a reason for hiding this comment

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

It looks to me like this line is the cause of the change in behaviour CP emailed a few people about this morning: the header on a container used to follow the user down the container, keeping (for example) the launch button easy to access when the top of the container has scrolled offscreen, and after this change it doesn't do that any more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Emily, small change that was missed in review! I've put out a fix :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks both, It’s great to see the solution in my absence on Friday, I noticed that this fix had an unintended side effect in other editions in the Fronts,

We’ve previously considered these editions as not in use, but involvement in data curation raises an open question about whether they are still actively use via fronts tool?

I’d like to clarify this to ensure we handle them appropriately.
Looking forward to your thoughts @fredex42 @Georges-GNM

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.

5 participants