-
Notifications
You must be signed in to change notification settings - Fork 4
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
Show status of auto updating lists #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good.
There is one assertion in the updated
status text that I wanted to clarify, regarding which titles are eligible when a list update is run.
I also suggested some possible UI text rewording for consistency, but they are not blockers for me.
if (!listId) { | ||
autoUpdateStatusName = "New"; | ||
autoUpdateStatusDescription = | ||
"The system will begin to populate this list using the configured search criteria when it is saved, and will fully populate the list during the first scheduled update that occurs after it has been saved."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: the comma is not needed in this sentence.
...or...
possible rewording: "This is a new list. Once the initial search criteria have been saved, the system will begin to populate its entries; however, the list might not be fully populated until the next scheduled update."
if (autoUpdateStatus === "init") { | ||
autoUpdateStatusName = "Initializing"; | ||
autoUpdateStatusDescription = | ||
"This list was created recently. The system has partially populated the list using the configured search criteria, and will fully populate the list during the next scheduled update."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: comma not needed in second sentence.
...or...
possible rewording: "There are unsaved changes to the search criteria for this list. Once the changes have been saved, the new search criteria will be used to repopulate the list during the next scheduled update."
} else if (!autoUpdateStatus) { | ||
autoUpdateStatusName = "Changing to automatic"; | ||
autoUpdateStatusDescription = | ||
"This list was populated manually, and is being changed to be updated automatically. The system will repopulate the list using the configured search criteria during the first scheduled update that occurs after it has been saved."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: comma not needed in first sentence.
...or...
possible rewording: "This list was populated manually, but is being changed to be updated automatically. The configured search criteria will be used to repopulate the list during the next scheduled update."
} | ||
/> | ||
{autoUpdate && ( | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. I didn't know this Fragment
shorthand. Had me confused for a minute.
} else if (autoUpdateStatus === "repopulate") { | ||
autoUpdateStatusName = "Repopulating"; | ||
autoUpdateStatusDescription = | ||
"The search criteria for this list were changed recently, but the entries have not yet been updated. The system will repopulate the list using the current search criteria during the next scheduled update."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible rewording: "The search criteria for this list were changed recently, but the entries have not yet been updated. The new search criteria will be used to repopulate the list during the next scheduled update."
} else if (autoUpdateStatus === "updated") { | ||
autoUpdateStatusName = "Updated"; | ||
autoUpdateStatusDescription = | ||
"This list was fully populated during the last scheduled update, using the configured search criteria and the titles that were available at the time. Titles that have been acquired since the last update will be added to the list during the next scheduled update."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the first sentence is entirely accurate: Are all available titles considered when the list is updated? Or just ones that have come in since the last update time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it depends on which update. If it's the first one, then all titles are considered. After that, only the new titles are considered. In either case, it should result in the list being updated to reflect all the titles that are available at the time. I was trying to describe that without getting too far into the details, but I'm open to rewordings.
} else if (autoUpdateStatus === "updated") { | ||
autoUpdateStatusName = "Updated"; | ||
autoUpdateStatusDescription = | ||
"This list was fully populated during the last scheduled update, using the configured search criteria and the titles that were available at the time. Titles that have been acquired since the last update will be added to the list during the next scheduled update."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible rewording of second sentence: "New titles matching the criteria will be added to the list during the next scheduled update."
src/reducers/customListEditor.ts
Outdated
@@ -703,6 +716,22 @@ const isSearchModified = (state: CustomListEditorState): boolean => { | |||
return baselineSearchUrl !== currentSearchUrl; | |||
}; | |||
|
|||
/** | |||
* Checks if a the search parameters in a custom list editor state have been modified, and stores |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
- extra word: "Checks if [a] the search..."
- unneeded comma: "... have been modified[,] and stores..."
Might also be helpful to include the list of statuses in the main PR comment, since some with access to this repo will not have access to Notion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚀
Description
This modifies the UI so that users can see the status of auto updating lists.
auto_update_status
value returned from the CM, as well as the local modification state of the search parameters in the list editor.isSearchModified
property in the Redux state for the list editor. Previously, there was only anisModified
property, which tracked changes to any part of the list, possibly including the search parameters. TheisSearchModified
property now separately tracks only the changes to search parameters, and theisModified
computation now uses the value ofisSearchModified
instead of checking for changes to search parameters itself.The possible statuses are:
When a new list is being created:
When a saved list has
auto_update_state
=init
:When a saved list has
auto_update_state
=repopulate
:When a saved list has
auto_update_state
=updated
:When a saved list has unsaved changes to its search parameters (regardless of
auto_update_state
)Motivation and Context
This makes it easier for users to understand how auto updating lists work, and why entries in a list may not match the current search results.
Notion: https://www.notion.so/lyrasis/Implement-Admin-UI-changes-for-auto-updating-lists-a65aeb9b85214073ace9818deb04a17e
How Has This Been Tested?
Checklist: