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

Add displayId virtual field to activity reports #162

Merged
merged 8 commits into from
Feb 16, 2021

Conversation

jasalisbury
Copy link

Description of change

Add a human friendly report ID field to activity reports

How to test

  1. Pull down
  2. Retrieve an activity report (curl http://localhost:3000/api/activity-reports/1). Display ID should look something like R01-AR-1

Issue(s)

Checklist

  • Meets issue criteria
  • Code tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • Documentation updated

Copy link
Collaborator

@dcloud dcloud left a comment

Choose a reason for hiding this comment

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

Codewise this looks fine, though I'm confused about the human-friendly identifiers. My (limited) understanding was that the 'R00-AR0' style identifier was used in SmartSheet and the plan was to use a different style for new identifiers. Further, old identifiers were going to be imported into a "legacy id" column and used to alias to the new id number. If we added this virtual field, wouldn't we be adding a third identifier that end users might see?

@dcloud dcloud mentioned this pull request Feb 11, 2021
4 tasks
@jasalisbury
Copy link
Author

Codewise this looks fine, though I'm confused about the human-friendly identifiers. My (limited) understanding was that the 'R00-AR0' style identifier was used in SmartSheet and the plan was to use a different style for new identifiers. Further, old identifiers were going to be imported into a "legacy id" column and used to alias to the new id number. If we added this virtual field, wouldn't we be adding a third identifier that end users might see?

I'm not 100% sure what the legacy ID will be used for. My understanding from this slack conversation https://gsa-tts.slack.com/archives/C017RJ4GEET/p1612903356126900 is the legacy ID will be used to pull up old activity reports. Thoughts @rahearn?

@rahearn
Copy link

rahearn commented Feb 11, 2021

the legacy ID will be used to pull up old activity reports

This is correct. The issue with the old reports is that every region had their own sheet, so there are duplicate numeric IDs. We can't just renumber them all when we import, because the old IDs may be referenced in narrative in other reports (a not-uncommon occurrence in the system in use last year).

I suppose we could get rid of the legacy id column if we were able to use a compound primary key to get around the uniqueness issues, but that seems like over-engineering the system to deal with just these reports that will become less and less of an issue as these imported reports are describe activities further and further in the past.

@rahearn
Copy link

rahearn commented Feb 11, 2021

This comment is relevant to this PR

Copy link
Collaborator

@kryswisnaskas kryswisnaskas left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

This code looks good, though it feels like this code should be accompanied by an api doc update and an update to remove the ID generation from the frontend code and use this field instead.

@jasalisbury
Copy link
Author

This code looks good, though it feels like this code should be accompanied by an api doc update and an update to remove the ID generation from the frontend code and use this field instead.

I'll update the API docs. Frontend usage of the new field is happening in this PR

Copy link

@rahearn rahearn left a comment

Choose a reason for hiding this comment

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

I'll update the API docs. Frontend usage of the new field is happening in this PR

awesome, thanks!

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.

5 participants