Circulars Archive Group View #2617

Courey · 2024-10-10T19:25:12Z

This work is based off #2538 and will be turned into a non-draft PR once that work has been merged in and I rebase off main.

Description

This work includes:

a button to toggle from the index view to the group view
a drop down list of the circular titles when viewed from the circulars archive
a link to each circular group in the circulars archive group view
a route for a synonym group overview
full circular data in a drop down on the group overview page with a link to each individual circular
a feature flag hiding the synonyms work

Related Issue(s)

Resolves #2544

Testing

This has been thoroughly manually tested locally, but should be tested on dev WITHOUT the feature flag on to ensure that no existing functionality has broken.

Images

codecov · 2024-10-10T19:30:06Z

Codecov Report

Attention: Patch coverage is 8.82353% with 93 lines in your changes missing coverage. Please review.

Project coverage is 6.11%. Comparing base (1986a6a) to head (9f33d24).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
app/routes/circulars._archive._index/route.tsx	0.00%	27 Missing ⚠️
app/routes/circulars.events.$slug.tsx	0.00%	24 Missing ⚠️
app/routes/synonyms/synonyms.server.ts	34.78%	15 Missing ⚠️
app/routes/circulars.events/route.tsx	0.00%	9 Missing ⚠️
...es/circulars._archive._index/SynonymGroupIndex.tsx	0.00%	7 Missing ⚠️
app/routes/synonyms._index.tsx	0.00%	3 Missing ⚠️
app/components/pagination/Pagination.tsx	0.00%	2 Missing ⚠️
.../routes/circulars.$circularId.($version)/route.tsx	0.00%	2 Missing ⚠️
...es/circulars.edit.$circularId/CircularEditForm.tsx	0.00%	2 Missing ⚠️
app/components/circularDisplay/FrontMatter.tsx	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2617   +/-   ##
=====================================
  Coverage   6.10%   6.11%           
=====================================
  Files        168     171    +3     
  Lines       4258    4333   +75     
  Branches     468     475    +7     
=====================================
+ Hits         260     265    +5     
- Misses      3996    4066   +70     
  Partials       2       2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lpsinger

Please rebase.

Courey · 2024-10-16T15:49:26Z

Added "Open/Close All" button on group overview page as requested by @jracusin

Courey · 2024-10-16T15:53:25Z

Please rebase.

That was the plan as I said in the original PR description:

This work is based off #2538 and will be turned into a non-draft PR once that work has been merged in and I rebase off main.

I rebased and took the PR out of draft status.

lpsinger

There are more conflicts. Please rebase again.

lpsinger

First, some high-level UX feedback:

Please use numbered lists, not bulleted lists, for Circulars in the index view. The list number value should be the Circular number.
For now, don't have disclosure arrows on the per-group page. Just display all of the Circulars belonging to the group. We can fine-tune the UI to quickly navigate within a group in a future PR.

Courey · 2024-10-22T13:37:41Z

Removed details element from group overview:

Made ul an ol with circularId values:

lpsinger · 2024-10-22T16:26:01Z

app/components/circularDisplay/AstroDataContext.tsx

@@ -29,6 +29,7 @@ export const AstroDataContext = createContext<AstroDataContextProps>({})
 /**
 * An Astro Flavored Markdown enriched link.
 */
+// eslint-disable-next-line react/display-name


Why was this addition necessary?

when I moved the component into the components directory, it is judged as a component definition and causes this warning:

I was trying to make the move have no changes to the code if possible

app/components/pagination/PaginationSelectionFooter.tsx

lpsinger · 2024-10-22T16:28:38Z

app/components/pagination/PaginationSelectionFooter.tsx

@@ -53,6 +64,7 @@ export default function PaginationSelectionFooter({
            page={page}
            limit={limit}
            totalPages={totalPages}
+            view={view}


Why does the Pagination component need to know about the view?

because in getPageLink if the view is not set as a search param, then when it creates the link, it doesn't include view. If it doesn't include view, then the view defaults to index. If it's always index, then the pagination links will never work for groups. To show you what I mean, here is what happens when I remove the view from getPageLinks. When I hover over the 2 button, this is the link. You will see that it doesn't include the view so when that page is navigated to, it's the index view which is the default.

Ah, I see.

The last time that I looked at this component, I noticed that a lot of apparently separate concerns were leaking into it from pages that use it. At some point, I'd like to come back to this and try to refactor it.

I think that's a good idea. It did feel odd to have to add view specific code to it.

app/routes/synonyms.$synonymId.tsx

lpsinger · 2024-10-22T18:48:49Z

Removed details element from group overview:

Made ul an ol with circularId values:

Instead of the boxes, could we just put a horizontal rule between Circulars for now?

app/routes/group.$synonymId.tsx

app/routes/group/route.tsx

app/routes/synonyms/synonyms.server.ts

lpsinger

Please place the new event pages under /circulars/group/ rather than /group/.
The synonym IDs make for URLs that are not very presentable. Instead, can we use a slugified form of the event name (any of the event names that belong to the group) as the path component?

app/routes/circulars._archive._index/SynonymGroupIndex.tsx

app/routes/group.$synonymId.tsx

Courey · 2024-11-12T16:04:22Z

The synonym IDs make for URLs that are not very presentable. Instead, can we use a slugified form of the event name (any of the event names that belong to the group) as the path component?

I have a couple of concerns about this if I'm understanding correctly.

It doesn't seem very RESTful. the id of the group would be the synonymId, not the eventId (because sometimes it is comprised of more than one eventId). it is the synonym we are representing here, not the event.
Let's say we had GRB 111224A, GRB 111224B, and GRB 111224C. If we slugified that into something like GRB_111224A-GRB_111224B-GRB_111224C if a user bookmarked that uri and later a moderator decided that GRB 111224C was actually an unrelated event and removed that from the group, then the users bookmarked uri wouldn't work anymore. It is fragile.
We could handle for that so it just picks one eventId and pulls up the record based on that, if the group that eventId was in changed, it would pull up a totally different group than originally. so circulars/group/GRB_111224C would now bring up just that record instead of the original group it belonged to before a moderator changing it. So it wouldn't go to the same group as when the bookmark/link was created.
If we disregarded any concerns about uri fragility or possibly pulling up the wrong record than intended, it also changes how things work on the back end:
- we would have to parse the slug which isn't a huge deal, but it's not as terse as just looking up by synonymId.
- If we slugified all the eventIds in the group, we would be able to get the records based on those eventIds. doing it that way may include records from a different group as it's not specifying the synonymId.
- if we try to handle that by checking to see if the records have the same synonymId, and we have two records and they both have different synonymIds, which synonymId wins out?
- if we accepted just one eventId of the group, we'd have to make a db call to get the synonymId for that eventId, then using those results, we'd have to make another call to get all the other eventIds with that syonymId. So it increases queries.

Can you explain what about uuids feels not presentable to you?

lpsinger · 2024-11-12T19:01:39Z

The synonym IDs make for URLs that are not very presentable. Instead, can we use a slugified form of the event name (any of the event names that belong to the group) as the path component?

I have a couple of concerns about this if I'm understanding correctly.

It doesn't seem very RESTful. the id of the group would be the synonymId, not the eventId (because sometimes it is comprised of more than one eventId). it is the synonym we are representing here, not the event.

Let's say we had GRB 111224A, GRB 111224B, and GRB 111224C. If we slugified that into something like GRB_111224A-GRB_111224B-GRB_111224C if a user bookmarked that uri and later a moderator decided that GRB 111224C was actually an unrelated event and removed that from the group, then the users bookmarked uri wouldn't work anymore. It is fragile.

We could handle for that so it just picks one eventId and pulls up the record based on that, if the group that eventId was in changed, it would pull up a totally different group than originally. so circulars/group/GRB_111224C would now bring up just that record instead of the original group it belonged to before a moderator changing it. So it wouldn't go to the same group as when the bookmark/link was created.

If we disregarded any concerns about uri fragility or possibly pulling up the wrong record than intended, it also changes how things work on the back end:

we would have to parse the slug which isn't a huge deal, but it's not as terse as just looking up by synonymId.

If we slugified all the eventIds in the group, we would be able to get the records based on those eventIds. doing it that way may include records from a different group as it's not specifying the synonymId.

if we try to handle that by checking to see if the records have the same synonymId, and we have two records and they both have different synonymIds, which synonymId wins out?

if we accepted just one eventId of the group, we'd have to make a db call to get the synonymId for that eventId, then using those results, we'd have to make another call to get all the other eventIds with that syonymId. So it increases queries.

I proposed using "any of the event names that belong to the group" as the path component. So /circulars/groups/GW170817, /circulars/groups/GRB170817A, /circulars/groups/GW170817+GRB170817A would all represent the same resource. (One would need to be reported as the canonical URL for SEO purposes; probably the version with the maximal set of terms.)

Another option (but not my preference) would be to do what Stack Overflow does: have an opaque UUID followed by a slug that is ignored. So for example, all of the following refer to the same resource:

although the first is canonical.

Can you explain what about uuids feels not presentable to you?

It's a usability issue: https://en.wikipedia.org/wiki/Clean_URL

Courey · 2024-11-14T15:09:05Z

I proposed using "any of the event names that belong to the group" as the path component. So /circulars/groups/GW170817, /circulars/groups/GRB170817A, /circulars/groups/GW170817+GRB170817A would all represent the same resource. (One would need to be reported as the canonical URL for SEO purposes; probably the version with the maximal set of terms.)

That still does not solve for or answer the issues I brought up in the above comment. If we take your example of /circulars/groups/GW170817, /circulars/groups/GRB170817A, /circulars/groups/GW170817+GRB170817A. Jane Doe is interested in this grouping that she discovered when searching for GRB170817A. She bookmarks /circulars/groups/GRB170817A so she can go back and look at this group. A moderator looks at the grouping and decides that while the events were very close together, they are actually different events. So the moderator removes GRB170817A from the group with GRB170817. So now when Jane visits the website looking for the information from both GRB170817A and GRB170817, the link she bookmarked would take her to the group that only has GRB170817A in it.

This request would be like removing the circularId from the circulars path and replacing it with the circular subject instead. But you shouldn't do that because the subject could be edited and if so, it would break links because it's fragile.

The only difference in these patterns is UUID vs integer id. But the integer ID isn't giving any additional information in a human readable format. I have no insight into what circular 12345 is about.

To make it restful, it should follow the pattern /{resource}/{id}

https://restfulapi.net/resource-naming/

lpsinger · 2024-11-14T15:56:36Z

I don't disagree that RESTfulness is a desirable property for URL routing, but an entity need not have a unique URL, does it? By analogy, files on a filesystem don't have unique paths.

It's true that we are using Circular IDs as URL path components, but Circular IDs are part of the publicly-visible bibliographic record of Circulars. UUIDs are an internal detail.

Courey · 2024-11-14T20:54:22Z

I disagree that what you are proposing is "clean".
from your wiki link on clean urls:

Other reasons for using clean URLs include search engine optimization (SEO),[1] conforming to the representational state transfer (REST) style of software architecture, and ensuring that individual web resources remain consistently at the same URL. This makes the World Wide Web a more stable and useful system, and allows more durable and reliable bookmarking of web resources

your request violates:

conforming to the representational state transfer (REST) style of software architecture

because calling a resource by one of it's members is not restful. not restful != clean

and

ensuring that individual web resources remain consistently at the same URL. This makes the World Wide Web a more stable and useful system, and allows more durable and reliable bookmarking of web resources

because of the situations outlined above when the members of the group change. If I have a group of GRB A, GRB B, GRB C, and GRB D. If I bookmark circulars/groups/GRB_D it it had been removed, it would take me to the new group for GRB D without any reference to why GRB A, GRB B, and GRB C that were there before are now missing. This is not a durable uri because it is based on changeable values. One would instead expect to visit the group bookmarked originally and see that GRB D was no longer there, then one could search for GRB D to see what new group it was part of. Just having it jump to the new group with no indication of why the new group is not the old group is not an anticipated behavior and it does not make for reliable bookmarking. Since the slugs would be based on changeable values, bookmarks would always be fragile which is also not clean.

Additionally, is it really worth the complexity that it creates?
things that would have to be accounted for:

slugging and de-slugging singular eventIds
handling slugging and de-slugging more than 1 eventId
looking for a synonym group by getting the synonym off the de-slugified eventId and then making a second query for the members of the group by the results of that query.
keeping track of SEO for changing group members

lpsinger · 2024-11-15T13:48:48Z

In a URL like /circulars/group/GW170817, the "group" component is a the resource and "GW170817" is the ID. I view the event names as permanent, stable IDs. The UUIDs are not.

Although this approach appears RESTful to me, I don't want to get bogged down in that argument, because this path is not an API endpoint and it is not all that important whether it is RESTful or not. It's more important that they are human-readable and predictable.

Here are some examples of style guidelines and advice related to human-readable URLs:

Additionally, is it really worth the complexity that it creates?
things that would have to be accounted for:

slugging and de-slugging singular eventIds

handling slugging and de-slugging more than 1 eventId

looking for a synonym group by getting the synonym off the de-slugified eventId and then making a second query for the members of the group by the results of that query.

keeping track of SEO for changing group members

That looks like a reasonable implementation plan. Is there a good open-source slugging library we could use?

Courey · 2024-12-03T15:05:51Z

Additional work for this feature:

add slug field to dynamodb record (all lowercase with dashes)
add index to the slug field
add code to save slug in any place that a synonym could be created or updated (using github slugger package)
create function to get synonym by slug
update path to be /circulars/events/<slug>
update sandbox-seed to reflect desired functionality
create additional ticket to backfill any existing synonyms' slugs
update existing ticket and code for synonym backfill to include slug

lpsinger · 2024-12-04T18:47:27Z

This is great, @Courey. I am swamped until the end of the day but I'll try to post a review by tomorrow. Meanwhile, @dakota002, would you please give it a once over?

Courey · 2024-12-04T18:50:14Z

I'm currently fixing tests. You are welcome to review the application code while I sort those out, but there could be changes as a result of fixing those tests.

lpsinger · 2024-12-04T18:51:39Z

I'm currently fixing tests. You are welcome to review the application code while I sort those out, but there could be changes as a result of fixing those tests.

Perfect. Just ping us when you're ready. I won't bother you until then!

Courey · 2024-12-06T17:36:00Z

found a small bug on the moderator view that was introduced by the changes in this branch. fixing that now.

__tests__/synonyms.server.ts

app/routes/synonyms/synonyms.lib.ts

app/routes/synonyms/synonyms.server.ts

app/routes/circulars.events.$slug.tsx

app/routes/circulars._archive._index/route.tsx

adds validation and error handling fixes tests adds modal warning prior to delete changes removal button to words instead of only icons code review change requests removing unused className formatting autofill moderator synonym eventId selector adding create sad path test removing feature flag check adding a 3 second debounce Adds grouped view to circulars archive index adds missing route and flag checks

lpsinger · 2024-12-10T02:35:49Z

app/routes/circulars._archive._index/route.tsx

+  const { query, startDate, endDate, sort, view } = handleSearchParams(
+    searchParams,
+    synonymFlagIsOn
+  )


Please undo this refactoring.

Refactoring to make code more readable is a good thing, but this refactoring is not related to the objectives of this PR. Please don't mix up refactoring with feature or bug fix PRs. It increases the review burden because I have to check that the refactoring itself is correct.

I will undo it, but in the software industry there is something called the campfire rule. This means that if your feature touches existing code, refactoring for readability, performance, etc is encouraged. Not making improvements when working in an area is unusual for most engineers, so that might be something we want to put in the contributing guide or wherever we put our team norms.

These are good improvements! But please do the refactoring in a separate PR. I would like to review the refactoring itself independently from the functional changes.

When you are working on a new feature or a bug fix and you notice a helpful refactoring, please do the refactoring in a separate commit and open a separate PR for it.

lpsinger · 2024-12-10T02:37:47Z

app/routes/circulars._archive._index/route.tsx

+    page,
+    ...results,
+    requestedChangeCount,
+    synonymFlagIsOn,


No need to pass this through the loader. This value is already available in components through the useFeature hook.

Courey · 2024-12-10T14:45:56Z

Is the review complete? I would like to make all requested changes at once because I do a thorough testing locally to ensure none of the refactoring breaks anything. When we do the review in review after review, it eats up a lot of time with the testing because I have to retest all of it each time. I don't want to skip testing because since we merge each others branches, I want to make sure it is completely tested each time I push in case it gets merged in.

lpsinger · 2024-12-10T14:56:43Z

Is the review complete?

I'm done. I have some look-and-feel requests about the view toggle button, but I had planned to do that in a separate issue after we merge this.

lpsinger requested changes Oct 16, 2024

View reviewed changes

lpsinger mentioned this pull request Oct 16, 2024

index without working links or create button #1722

Closed

Courey force-pushed the courey/grouping branch 2 times, most recently from e64cd81 to d91fa13 Compare October 16, 2024 15:00

Courey marked this pull request as ready for review October 16, 2024 15:46

Courey requested a review from lpsinger October 16, 2024 15:46

Courey requested a review from dakota002 October 16, 2024 15:54

lpsinger requested changes Oct 16, 2024

View reviewed changes

Courey force-pushed the courey/grouping branch from f425a78 to e53c732 Compare October 17, 2024 13:39

Courey requested a review from lpsinger October 17, 2024 13:39

lpsinger requested changes Oct 18, 2024

View reviewed changes

Courey requested a review from lpsinger October 22, 2024 13:44

lpsinger requested changes Oct 22, 2024

View reviewed changes

Courey requested a review from lpsinger October 22, 2024 18:03

lpsinger requested changes Oct 23, 2024

View reviewed changes

Courey requested a review from lpsinger October 24, 2024 15:49

lpsinger requested changes Nov 8, 2024

View reviewed changes

app/routes/circulars._archive._index/SynonymGroupIndex.tsx Outdated Show resolved Hide resolved

app/routes/group.$synonymId.tsx Outdated Show resolved Hide resolved

app/routes/group.$synonymId.tsx Outdated Show resolved Hide resolved

Courey requested a review from lpsinger November 12, 2024 16:05

Courey force-pushed the courey/grouping branch from 81d7ea6 to 772831d Compare November 12, 2024 16:07

Courey force-pushed the courey/grouping branch from 772831d to 71cdc0d Compare December 4, 2024 18:44

Courey mentioned this pull request Dec 4, 2024

backfill slugs on existing synonym records #2750

Open

Courey force-pushed the courey/grouping branch from cb9955b to 5da3e58 Compare December 6, 2024 15:56

lpsinger requested changes Dec 8, 2024

View reviewed changes

Courey added 14 commits December 9, 2024 12:20

adding open all button

fe52f52

removing unused interface

357e603

removing details on overview, making ul an ol with circularIds

63acb56

review feedback

b8656d3

using different status codes and code formatting

98f59d2

changing route

48c4104

use slugs instead of ids

9c626cb

updating tests to include slugs

bb74a69

adding copyright

14bf9fa

update pagination in moderation

a9e81f6

code review revisions

c6236be

using useSearchParams

307758d

removing unused function

2dae781

Courey force-pushed the courey/grouping branch from 34b9bc1 to 2dae781 Compare December 9, 2024 18:39

lpsinger requested changes Dec 10, 2024

View reviewed changes

undoing refactoring and using feature hook

9f33d24

lpsinger approved these changes Dec 10, 2024

View reviewed changes

lpsinger merged commit 928b1e6 into nasa-gcn:main Dec 10, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circulars Archive Group View #2617

Circulars Archive Group View #2617

Courey commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

lpsinger left a comment

Courey commented Oct 16, 2024

Courey commented Oct 16, 2024

lpsinger left a comment

lpsinger left a comment

Courey commented Oct 22, 2024

lpsinger Oct 22, 2024

Courey Oct 22, 2024

Courey Oct 22, 2024

lpsinger Oct 22, 2024

Courey Oct 22, 2024

lpsinger Oct 23, 2024

Courey Oct 23, 2024

lpsinger commented Oct 22, 2024

lpsinger left a comment •

edited by Courey

Loading

Courey commented Nov 12, 2024

lpsinger commented Nov 12, 2024

Courey commented Nov 14, 2024

lpsinger commented Nov 14, 2024

Courey commented Nov 14, 2024

lpsinger commented Nov 15, 2024

Courey commented Dec 3, 2024 •

edited

Loading

lpsinger commented Dec 4, 2024

Courey commented Dec 4, 2024

lpsinger commented Dec 4, 2024

Courey commented Dec 6, 2024

lpsinger Dec 10, 2024

Courey Dec 10, 2024

lpsinger Dec 10, 2024

lpsinger Dec 10, 2024

Courey commented Dec 10, 2024

lpsinger commented Dec 10, 2024

Circulars Archive Group View #2617

Circulars Archive Group View #2617

Conversation

Courey commented Oct 10, 2024 • edited Loading

Description

Related Issue(s)

Testing

Images

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

lpsinger left a comment

Choose a reason for hiding this comment

Courey commented Oct 16, 2024

Courey commented Oct 16, 2024

lpsinger left a comment

Choose a reason for hiding this comment

lpsinger left a comment

Choose a reason for hiding this comment

Courey commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpsinger commented Oct 22, 2024

lpsinger left a comment • edited by Courey Loading

Choose a reason for hiding this comment

Courey commented Nov 12, 2024

lpsinger commented Nov 12, 2024

Courey commented Nov 14, 2024

lpsinger commented Nov 14, 2024

Courey commented Nov 14, 2024

lpsinger commented Nov 15, 2024

Courey commented Dec 3, 2024 • edited Loading

lpsinger commented Dec 4, 2024

Courey commented Dec 4, 2024

lpsinger commented Dec 4, 2024

Courey commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Courey commented Dec 10, 2024

lpsinger commented Dec 10, 2024

Courey commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

lpsinger left a comment •

edited by Courey

Loading

Courey commented Dec 3, 2024 •

edited

Loading