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.

Sign up for GitHub

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

Jump to bottom

TNO-1136: User can set toning #2226

Closed
wants to merge 1 commit into from
Closed

TNO-1136: User can set toning #2226

wants to merge 1 commit into from

Conversation

areyeslo
Copy link
Collaborator

@areyeslo areyeslo commented Aug 28, 2024 •
edited
Loading

Feature Update: User-Defined Tone Assessment for Articles

This update introduces the ability for subscribers to assess and set a personal tone for an article, independent of the original tone. The custom tone selected by the user will be saved and associated with their account, ensuring that their personal assessment is retained without altering the article's original tone data.

Implementation Details:

  • Component Reuse: The FormikSentiment component has been modified to match the UI in subscriber.

Example:
image

DB:

content.versions:

{"3": {"Body": "<p>Test Body change</p>", "Page": "1", "Tone": -3, "Byline": "John Duncaster", "Edition": "", "OwnerId": 0, "Section": "", "Summary": "<p>Event</p>", "Headline": "Amazing Event on School", "IsPrivate": false, "SourceUrl": ""}}

select c.id, c.status, c.headline, c.body, c.byline, ct.tone_pool_id, ct.value
from "content" c, content_tone ct
where c.id = 3204 and
ct.content_id = c.id

id  |status|headline                     |body                                 |byline|tone_pool_id|value|
----+------+-----------------------------+-------------------------------------+------+------------+-----+
3204|     2|July 24th - Testing on Quadra|<p>July 24th - Testing on Quadra.</p>|      |           1|    4|

Story: https://citz-gdx.atlassian.net/browse/TNO-1136

@areyeslo areyeslo self-assigned this Aug 28, 2024
@areyeslo areyeslo added enhancement New feature or request subscriber PR contains changes towards the subscriber application, tno-core update Indicates that there have been changes to our tno-core package, which can pose concurrency issues. labels Aug 28, 2024
@areyeslo areyeslo requested a review from Fosol August 28, 2024 21:42
Fosol
Fosol requested changes Aug 28, 2024
View reviewed changes
Copy link
Collaborator

@Fosol Fosol left a comment

Choose a reason for hiding this comment

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

This solution lacks a few key features.

  1. None of the reports will show the user's sentiment
  2. The user cannot select an appropriate tone pool to use

app/subscriber/src/features/my-reports/edit/validation/SentimentFormSchema.ts
export const sentimentFormSchema = object().shape({
tonePools: array().of(
object().shape({
value: number().required('Tone value is required'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't believe tone is required. Did Scott ask for this?

app/subscriber/src/features/my-reports/edit/content/ContentEditForm.tsx
@@ -215,7 +215,14 @@ export const ContentEditForm = React.forwardRef<HTMLDivElement | null, IContentE
</Col>
<Col className="sentiment">
<Sentiment
value={form.content.tonePools.length ? form.content.tonePools[0].value : undefined}
value={
form.content.versions?.[userId]?.tone !== undefined &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell tone should never be null

Copy link
Collaborator Author

@areyeslo areyeslo Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

First time the tone can be null when the user did not save her/his tone and until tone is saved by the user, then we have a value.

app/subscriber/src/features/my-reports/edit/content/stories/ContentForm.tsx
: content.body,

const sentiment =
content.versions?.[userId]?.tone !== undefined && content.versions?.[userId]?.tone != null
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell tone should never be null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First time the tone can be null when the user did not save her/his tone and until tone is saved by the user, then we have a value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it can be null then your interface is not correct. I would recommend updating the interface if you want to handle null. However, I don't know why you would want both null and undefined.
This is one of the reasons I'm not a fan of javascript.

app/subscriber/src/features/my-reports/edit/content/stories/ContentForm.tsx
body:
content.versions?.[userId]?.body ??
(isAV ? (content.isApproved && content.body ? content.body : content.summary) : content.body),
tonePools:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you using a property tonePools here? The model is for IContentVersionModel has a tone property you added.

app/subscriber/src/features/my-reports/edit/content/stories/ContentForm.tsx
(isAV ? (content.isApproved && content.body ? content.body : content.summary) : content.body),
tonePools:
sentiment !== undefined && sentiment !== null
? [{ value: sentiment, id: tonePools[0].id }]
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no reason to apply the wrong id to this user's tone`

app/subscriber/src/features/my-reports/edit/content/stories/ContentForm.tsx
@@ -152,6 +183,19 @@ export const ContentForm: React.FC<IContentFormProps> = ({
onContentChange?.(values);
}}
/>
<Formik initialValues={versions} validationSchema={sentimentFormSchema} onSubmit={() => {}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't create a sub-form simply to work around the behavior of the FormikSentiment. Make the component smarter.

app/subscriber/src/features/my-reports/edit/content/stories/ContentForm.tsx
<Formik initialValues={versions} validationSchema={sentimentFormSchema} onSubmit={() => {}}>
{() => (
<Form>
<FormikSentiment
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this component able to show the current value the user selected when the page is reloaded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Through using handleSentimentChange callback which is executed when the user clicks on the tone button.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That function saves the value. The component has no way to know the current value. If you refresh the page it wouldn't know how to get the value.

libs/net/entities/Models/ContentVersion.cs
@@ -82,6 +86,7 @@ public ContentVersion(Content content, User owner)
this.Body = content.Body;
this.SourceUrl = content.SourceUrl;
this.IsPrivate = content.IsPrivate;
this.Tone = content.TonePools.Count > 0 ? (int?)content.TonePools[0].Id : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This incorrect, it should be the TonePoolsManyToMany.FirstOrDefault().Value not the TonePool[0].Id.

libs/npm/core/src/components/formik/sentiment/FormikSentiment.tsx
const updatedTonePools = !!defaultTonePool
? [{ ...defaultTonePool, value: option }]
: [];
setFieldValue(name.toString(), updatedTonePools, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to modify the the formik context field and also call the onSentimentChange. You can pass the valid name to the component to control what is being updated. Or you can choose to only perform the onSentimentChange if it has been provided. Doing both is a bad component.

@jdtoombs
Copy link
Collaborator

There has been a few changes to tno-core so just make sure you fetch the latest when you are ready! I can publish for you when you need, just let me know

@areyeslo
Copy link
Collaborator Author

areyeslo commented Sep 4, 2024

I have to close this PR. Somehow the changes were lost, I had to retrieve using reflog and created another branch.

@areyeslo areyeslo closed this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fosol Fosol Fosol requested changes

Assignees

@areyeslo areyeslo

Labels
enhancement New feature or request subscriber PR contains changes towards the subscriber application, tno-core update Indicates that there have been changes to our tno-core package, which can pose concurrency issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@areyeslo @jdtoombs @Fosol