Feat/custom note url #1699

Nick0603 · 2021-06-19T23:57:20Z

Relatied issue: #1478

story:

A created note
Click "Custom Note Url" in Menu
Type url you like and submit.
The url is customized and sync to all collaborators in real time.

Details:

That's ok if you want to set custom url for many times.

/serial_number -> /custom1 -> /custom2 -> /custom3

If some users browse the past url, they will redirect to latest one automatically.

/serial_number -> /custom3
/custom1 -> /custom3
/custom2 -> /custom3

It will be blocked when the custom url is conflict with exist url.
But if the conflicted url is set by you in the past. You have right to set back.

/custom1 -> /custom2 -> /custom1

Only the owner of note can edit custom url.

lib/migrations/202106200000000-create-note-aliases-history.js

Yukaii · 2021-06-21T06:55:27Z

lib/note/service.js

+const { Note, ArchivedNoteAlias, sequelize } = require('../models')
+const realtime = require('../realtime/realtime')
+
+const forbiddenAlias = ['', 'new', 'me', 'history', '403', '404', '500', 'config']


@jackycute Also public/docs/*.md paths?

Also config.forbiddenNoteIDs

lib/note/service.js

jackycute · 2021-06-21T07:42:54Z

lib/models/note.js

@@ -223,6 +228,18 @@ module.exports = function (sequelize, DataTypes) {

  Note.parseNoteId = function (noteId, callback) {
    async.series({
+      parseNoteIdByArchivedAlias: function (_callback) {


I'm not totally sure about why we keep the previously used alias in track?
What if the archived alias you about to change has been used?

I'm not totally sure about why we keep the previously used alias in track?

My thought is to avoid some user saved the archived alias but can't connect the note.

What if the archived alias you about to change has been used?

The user just change one that not be used. Or we need to set the special and personal prefix for the alias?

To make this PR be more specific, it's better not to add this feature for redirecting previous alias to new one.
I suggest to remove this and by adding a notice alert area in the custom note url modal like: Notice: the previous custom note url will not be redirected to the new one, please take your own risk to change this. would help.

a60814billy

Hi @Nick0603,

Thanks for your contribution, there are some suggestions:

In general, we use strict mode in backend.
Do not mix different asynchronous paradigm in a procedure.
Maybe we can fill-in alias url in modal if the note already setup an alias url.
To avoid some vulnerability, we only support these character in customize url: lowercase
letters, decimal digits, hyphen, underscore. And need check user input is valid both frontend and backend. Do not auto correct or filter out the input.
Alias would be conflict with another nodeId, shortid

a60814billy · 2021-06-21T09:22:28Z

lib/note/service.js

+  return alias.replace(/( |\/)/, '')
+}
+
+const asyncGetNote = async (originAliasOrNoteId) => {


We already have getNoteById function in note/index.js

Because the getNoteById in note/index.js noget pure note row data.
I think controller or service should be choose the enough data to use.

But I also agree DRY is nice here, I will adjust it.

a60814billy · 2021-06-21T10:20:43Z

lib/note/index.js

@@ -8,6 +8,7 @@ const { newCheckViewPermission, errorForbidden, responseCodiMD, errorNotFound, e
 const { updateHistory, historyDelete } = require('../history')
 const { actionPublish, actionSlide, actionInfo, actionDownload, actionPDF, actionGist, actionRevision, actionPandoc } = require('./noteActions')
 const realtime = require('../realtime/realtime')
+const serv = require('./service')


rename service -> noteAlias, and consider using object destruction

I would like to make the service is for all note controller, so I think the naming noteAlias is not suitable.
And the service will contains the common logic in controller and easy for testing.

lib/note/service.js

Nick0603 · 2021-06-21T19:42:16Z

Hi @Nick0603,

Thanks for your contribution, there are some suggestions:

In general, we use strict mode in backend.

Do not mix different asynchronous paradigm in a procedure.

Maybe we can fill-in alias url in modal if the note already setup an alias url.

To avoid some vulnerability, we only support these character in customize url: lowercase
letters, decimal digits, hyphen, underscore. And need check user input is valid both frontend and backend. Do not auto correct or filter out the input.

Alias would be conflict with another nodeId, shortid

OK, I will add the missed'use strict' line in code
I don't really know the suggestion. Could you say more or give me a example.
Yes, now the code can do it.
OK
OK, I use parseNoteIdAsync to validate the alias inputed by user. And then alias will be check by all aliases for a noteId includes 'note id '、'note short id'、'public/docs/*.md'

Signed-off-by: nick.chen <[email protected]>

Co-authored-by: Yukai Huang <[email protected]> Signed-off-by: nick.chen <[email protected]>

Signed-off-by: nick.chen <[email protected]>

a60814billy · 2021-06-22T07:19:28Z

Do not mix different asynchronous paradigm in a procedure.

For example:

The code mix two asynchronous paradigm, async/await and Promise.

const conflictNote = await exports.getNote(alias)
  .catch((err) => { throw err })

If you want to handle error in async/await, using try...catch statement.

try {
  const conflictNote = await exports.getNote(alias)
} catch ((err) => {
  throw err
})

But, the catch statement just rethrow the error, we can remove it.

const conflictNote = await exports.getNote(alias)

Signed-off-by: nick.chen <[email protected]>

a60814billy · 2021-06-25T23:00:19Z

lib/note/index.js

+
+  if (!note) {
+    logger.error('update note alias failed: note not found.')
+    return res.status(500).json({ status: 'error', message: 'Internal Error' })


return 404 for note not found

a60814billy · 2021-06-25T23:02:50Z

public/js/index.js

@@ -1266,6 +1277,62 @@ $('#revisionModalRevert').click(function () {
  editor.setValue(revision.content)
  ui.modal.revision.modal('hide')
 })
+
+// custom note url modal
+const updateNoteUrl = (noteUrl = '') => {


rename: const updateNoteAliasUrl = (noteAliasUrl = '')
or const updateNoteCustomUrl = (noteCustomUrl = '')

a60814billy · 2021-06-25T23:07:46Z

public/js/index.js

+    return
+  }
+
+  updateNoteUrl(customUrl)


handle error in catch statement:

updateNoteUrl(customUrl) .then(({status}) => { .... }) .catch((err) => { if (err.status === 400) ..... ... showErrorMessage('Something wrong.') })

a60814billy · 2021-07-23T15:51:21Z

Big thanks @Nick0603

Nick0603 force-pushed the feature/custom-note-url branch from 8d6605f to 9ded54e Compare June 20, 2021 00:02

jackycute requested review from a60814billy and Yukaii and removed request for a60814billy June 20, 2021 09:57

Nick0603 force-pushed the feature/custom-note-url branch from 106e457 to d44be73 Compare June 20, 2021 18:27

Yukaii temporarily deployed to codimd-pr-1699 June 21, 2021 04:33 Inactive

Yukaii requested changes Jun 21, 2021

View reviewed changes

jackycute reviewed Jun 21, 2021

View reviewed changes

a60814billy requested changes Jun 21, 2021

View reviewed changes

a60814billy reviewed Jun 21, 2021

View reviewed changes

hackmdio deleted a comment from jackycute Jun 21, 2021

a60814billy reviewed Jun 21, 2021

View reviewed changes

lib/note/service.js Outdated Show resolved Hide resolved

Yukaii temporarily deployed to codimd-pr-1699 June 21, 2021 15:11 Inactive

Yukaii temporarily deployed to codimd-pr-1699 June 21, 2021 15:55 Inactive

Nick0603 force-pushed the feature/custom-note-url branch from 77c3b03 to 1ee19ed Compare June 21, 2021 19:40

Yukaii temporarily deployed to codimd-pr-1699 June 21, 2021 19:40 Inactive

Nick0603 and others added 5 commits June 22, 2021 03:46

create custom-note-url flow

1579f20

Signed-off-by: nick.chen <[email protected]>

fix linter error

dab0c66

Signed-off-by: nick.chen <[email protected]>

Update lib/migrations/202106200000000-create-note-aliases-history.js

dc9c98e

Co-authored-by: Yukai Huang <[email protected]> Signed-off-by: nick.chen <[email protected]>

Update lib/migrations/202106200000000-create-note-aliases-history.js

0777f2d

Co-authored-by: Yukai Huang <[email protected]> Signed-off-by: nick.chen <[email protected]>

Clean code

014790a

Signed-off-by: nick.chen <[email protected]>

Nick0603 force-pushed the feature/custom-note-url branch from 1ee19ed to 014790a Compare June 21, 2021 19:46

Yukaii temporarily deployed to codimd-pr-1699 June 21, 2021 19:46 Inactive

revmoe archivedAlias logic

d72d6a0

Signed-off-by: nick.chen <[email protected]>

Yukaii temporarily deployed to codimd-pr-1699 June 22, 2021 18:01 Inactive

a60814billy requested changes Jun 25, 2021

View reviewed changes

a60814billy changed the base branch from develop to feature/custom-note-url-base July 23, 2021 15:49

a60814billy merged commit 829c961 into hackmdio:feature/custom-note-url-base Jul 23, 2021

a60814billy mentioned this pull request Jul 23, 2021

Feature/custom note url base #1704

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/custom note url #1699

Feat/custom note url #1699

Nick0603 commented Jun 19, 2021

Yukaii Jun 21, 2021

jackycute Jun 21, 2021

a60814billy Jun 21, 2021

jackycute Jun 21, 2021

Nick0603 Jun 21, 2021 •

edited

Loading

jackycute Jun 21, 2021 •

edited

Loading

a60814billy left a comment •

edited

Loading

a60814billy Jun 21, 2021

Nick0603 Jun 21, 2021 •

edited

Loading

a60814billy Jun 21, 2021

Nick0603 Jun 21, 2021 •

edited

Loading

Nick0603 commented Jun 21, 2021

a60814billy commented Jun 22, 2021

a60814billy Jun 25, 2021

a60814billy Jun 25, 2021

a60814billy Jun 25, 2021

a60814billy commented Jul 23, 2021

Feat/custom note url #1699

Feat/custom note url #1699

Conversation

Nick0603 commented Jun 19, 2021

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

Nick0603 Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

jackycute Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

a60814billy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nick0603 Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nick0603 Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Nick0603 commented Jun 21, 2021

a60814billy commented Jun 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a60814billy commented Jul 23, 2021

Nick0603 Jun 21, 2021 •

edited

Loading

jackycute Jun 21, 2021 •

edited

Loading

a60814billy left a comment •

edited

Loading

Nick0603 Jun 21, 2021 •

edited

Loading

Nick0603 Jun 21, 2021 •

edited

Loading