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

Task: Content Spacing Correction #801

Merged
merged 10 commits into from
Oct 5, 2023
Merged

Conversation

abdullai-t
Copy link
Contributor

@abdullai-t abdullai-t commented Sep 21, 2023

Summary / Highlights

A few deployments ago, we found a way to resolve the inconsistent spacing in our content between the text on the admin portal and how it displays on the community portal. However, there are still many pieces of content in the database that continue to use the old fix (adding double spacing in the richText field on the admin portal). In this PR, a task has been created to generates a report and fix contents in the db that needs fixing.

Testing Steps (Provide details on how your changes can be tested)

  1. Open the admin portal.
  2. create a task and select Process Content Spacing as the automatic process.
  3. open your email for the report generated.
  4. Visit the community portal to observe the spacing correction in the descriptions of actions, events, and testimonials.

@abdullai-t abdullai-t changed the title Ta-inconsistent-spacing-fix Task: Content Spacing Correction Sep 21, 2023
src/task_queue/jobs.py Outdated Show resolved Hide resolved
@BradHN1

This comment was marked as resolved.

@abdullai-t abdullai-t requested a review from BradHN1 October 4, 2023 10:19
@BradHN1
Copy link
Contributor

BradHN1 commented Oct 4, 2023

Couple of minor things:

For teams, the responsibility community is in the field primary_community.
In the report, if no community is found (for any item), leave it blank instead of N/A

@BradHN1
Copy link
Contributor

BradHN1 commented Oct 4, 2023

When you added the 'task' argument: res = func(task)
it broke all the tasks except the one which expected that argument.

@BradHN1
Copy link
Contributor

BradHN1 commented Oct 4, 2023

Tried testing with my local database. First I ran the report, second I ran the fix.. Both with all communities selected. Third I ran the report again, and the report had the same number of instances. So I think the data wasn[t fixed. Did you test it this way?

@abdullai-t abdullai-t requested a review from BradHN1 October 5, 2023 14:57
@abdullai-t
Copy link
Contributor Author

@BradHN1 , I have address all the feedbacks.

@BradHN1
Copy link
Contributor

BradHN1 commented Oct 5, 2023

Have you tested this with your local database, and when you update the spacing and run the report again, does it show no occurances of the strings being replaced?

@abdullai-t
Copy link
Contributor Author

and when you update the spacing and run the report ag

Yes. it doesn't show already replaced content in the report

return instance.community.name if instance.community else ""
elif instance and hasattr(instance, "primary_community"):
return instance.primary_community.name if instance.primary_community else ""

Copy link
Contributor

@BradHN1 BradHN1 Oct 5, 2023

Choose a reason for hiding this comment

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

else return "N/A"
in case the model doesn't have a community

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

if count > 0:
if is_feature_enabled(instance):
auto_correct_spacing(instance, field_name, field_value)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the "else"
Have it always append the data for what is being changed, even if the feature is enabled



FEATURE_FLAG_KEY = "update-html-format-feature-flag"
PATTERNS = ["<p><br></p>", "<p>.</p>", "<p>&nbsp;</p>"]
Copy link
Contributor

@BradHN1 BradHN1 Oct 5, 2023

Choose a reason for hiding this comment

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

One more pattern to correct: "<br />."

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed and pushed

elif hasattr(instance, "primary_community") and instance.primary_community in enabled_communities:
return True
else:
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

If the model has a "community" or "primary_community" field, but that is not filled, and the feature flag is enabled, it should make the change. I can update the routine as follows:
if hasattr(instance, "community"): if not instance.community or instance.community in enabled_communities: return True elif hasattr(instance, "primary_community"): if not instance.primary_community or instance.primary_community in enabled_communities: return True return False

Copy link
Contributor

@BradHN1 BradHN1 left a comment

Choose a reason for hiding this comment

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

LGTM

@BradHN1 BradHN1 merged commit 3eb2ef6 into development Oct 5, 2023
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.

2 participants