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

SHS-5980: Dashboard: Notifications/Announcements to Site Editors #1745

Merged
merged 17 commits into from
Feb 28, 2025

Conversation

codechefmarc
Copy link
Collaborator

@codechefmarc codechefmarc commented Feb 10, 2025

Summary

  • Adds a new dashboard block to show announcements. Data comes from a Google Sheets document.

Need Review By (Date)

2025-02-12

Urgency

low

Steps to Test

  1. Login to the site as a Developer
  2. Edit the dashboard layout by going to the Dashboard (/admin/dashboard) and clicking on "+ Edit Layout"
  3. Click "Add Block"
  4. Add a block of type "HSDP Announcements"

Screenshot 2025-02-06 at 6 18 59 PM

  1. Click "Add Block"
  2. Verify there is data coming from the Google Sheet (specified in ClickUp) and the table is sorted by date (descending), and contains titles and descriptions.
  3. Verify that the titles are bolded and markdown links are converted to HTML

Screenshot 2025-02-06 at 6 21 53 PM

Review Tasks

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Are PHP functions and variables in snake_case and not camelCase?
  • Does Drupal code follow Drupal Coding Standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided?

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

@codechefmarc codechefmarc changed the title SHS-5980: announcements SHS-5980: Dashboard: Notifications/Announcements to Site Editors Feb 10, 2025
@ahughes3 ahughes3 temporarily deployed to Tugboat February 12, 2025 17:16 Destroyed
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@codechefmarc Looks pretty good, there are just a couple of things I want you to check:

  • Left a comment in the code about a duplicated function call
  • In my tests, the changes in the Google sheet were almost instantly available in the block. Can you please confirm that the cache is working as expected?

@ahughes3 ahughes3 temporarily deployed to Tugboat February 12, 2025 23:40 Destroyed
@codechefmarc
Copy link
Collaborator Author

@cienvaras - Fixed the duplicate call and reworked caching so the table data itself is cached for 2 minutes.

Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@codechefmarc Thanks for the fixes! I still have a question and a minor change request, please check the inline comments.

@ahughes3 ahughes3 temporarily deployed to Tugboat February 14, 2025 01:03 Destroyed
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@codechefmarc LGTM! Thanks for the fixes.

@ahughes3 Ready for you to review.

@cienvaras cienvaras assigned ahughes3 and unassigned codechefmarc Feb 19, 2025
@cienvaras cienvaras requested a review from ahughes3 February 19, 2025 17:45
@ahughes3
Copy link
Collaborator

@codechefmarc @dalin- In reviewing with the team we would like to remove the "Title" column and rename "Description" to "Update"

Date and Update should be the only columns that are displayed on the dashboard.

I've updated the sheet with the actual updates that we want to display.

On the Google Sheet In the cells that have links there is a message that says - "This cell's contents violate its validation rule", they display correctly on the dashboard but just wanted to see if this would be an issue

@dalin-
Copy link
Collaborator

dalin- commented Feb 21, 2025

@ahughes3

On the Google Sheet In the cells that have links there is a message that says - "This cell's contents violate its validation rule", they display correctly on the dashboard but just wanted to see if this would be an issue

That's a data validation rule to show a warning if the content is > 150 characters. AFAICT it should be showing you "Please keep content within 150 characters" rather than that very technical warning. Also, once you throw a link in there that'll easily eat up another 50 chars, so I increased it to 200.

@ahughes3 ahughes3 temporarily deployed to Tugboat February 21, 2025 23:17 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat February 25, 2025 20:30 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat February 25, 2025 21:41 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat February 26, 2025 00:09 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat February 26, 2025 00:13 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat February 26, 2025 16:47 Destroyed
@codechefmarc
Copy link
Collaborator Author

@ahughes3 - I've made those changes to the table and is viewable on Tugboat.

@ahughes3 ahughes3 assigned cienvaras and unassigned ahughes3 Feb 27, 2025
Copy link
Collaborator

@ahughes3 ahughes3 left a comment

Choose a reason for hiding this comment

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

approved to be merged into the main dashboard PR but left some additional notes in the PR

@ahughes3 ahughes3 requested a review from cienvaras February 27, 2025 20:24
@cienvaras cienvaras merged commit 7ffad47 into SHS-5913_Dashboard-initiative Feb 28, 2025
17 of 18 checks passed
@cienvaras cienvaras deleted the SHS-5980--announcements branch February 28, 2025 17:04
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.

4 participants