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

(backend) Feat/link audit #1910

Merged
merged 9 commits into from
Aug 19, 2022
Merged

(backend) Feat/link audit #1910

merged 9 commits into from
Aug 19, 2022

Conversation

gweiying
Copy link
Contributor

@gweiying gweiying commented Aug 9, 2022

Problem

We want to provide an audit trail for link creators, so that they can know who has changed what on their link, and when.

See Notion story for details on user journey.

We currently store this data in our url_histories table. We parse the url records and expose an endpoint for the frontend to retrieve the records.

Solution

  • Add repository layer to retrieve url records
  • Add service layer to parse url records to return pairwise change sets
  • Add controller and exposes endpoint /api/link-audit

For authenticated query:

GET /api/link-audit?url=<shortUrl>&limit=10&offset=10

Returns: (JSON) HttpStatus 200 for success

{
  changes: [ // ordered from most recent
    {
      "type": "update",
      "key": "state",
      "prevValue": "ACTIVE",
      "currValue": "INACTIVE",
      "updatedAt": "2022-04-06 09:15:26.272+00",
    },
    {
      "type": "update",
      "key": "longUrl",
      "prevValue": "https://google.com",
      "currValue": "https://twitter.com",
      "updatedAt": "2022-03-01 13:59:01.341+00',
    },
    {
      "type": "create",
      "key": "longUrl",
      "prevValue": "",
      "currValue": "https://abc.com",
      "updatedAt": "2022-01-30T04:45:55.204Z"
    }
  ],
  offset: 10, 
  limit: 10, 
  count: 13, // total count of results
}

changes is an array of type changeSets[]

Values:

  • type (string): One of 'create', 'update'
  • key (string): One of 'state','userEmail','longUrl'
  • prevValue (string): string representation of previous key value
  • currValue (string): string representation of current key value
  • updatedAt

Refer to Notion doc for technical design.

Tests

What tests should be run to confirm functionality?

Future work

Improving pagination
We use the simplest approach to pagination currently (via limit and offset). There are various ways to improve this:

  • primary-key-based offsets e.g. “where id < last_id_seen” in the query
  • ORDER BY id rather than something like ORDER BY created_at

The rationale for these optimizations are documented in the Notion technical document - we can do these optimisations at a later date if we evaluate that this is a performance issue.

@gweiying gweiying marked this pull request as ready for review August 10, 2022 06:13
@yong-jie
Copy link
Member

just dropping a quick ack here that i've started reviewing this PR, but will need a bit more time after the dust settles to finish 🙏

Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

Nice work! PR looks functionally complete; i just have some minor nits you could consider addressing, and some things i'd to hear your thoughts on.

src/server/modules/audit/LinkAuditController.ts Outdated Show resolved Hide resolved
src/server/modules/audit/services/LinkAuditService.ts Outdated Show resolved Hide resolved
src/server/modules/audit/interfaces/LinkAuditService.ts Outdated Show resolved Hide resolved
@gweiying gweiying requested a review from yong-jie August 18, 2022 08:50
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

lgtm! feel free to merge when ready~

@gweiying gweiying merged commit cb71932 into develop Aug 19, 2022
@yong-jie yong-jie deleted the feat/link-audit branch August 19, 2022 09:58
@jimvae jimvae mentioned this pull request Sep 2, 2022
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