search

ether / etherpad-lite

Etherpad: A modern really-real-time collaborative document editor.
http://docs.etherpad.org/
Apache License 2.0
16.06k stars 2.79k forks source link

Nested Pad support #4292

Closed HMarzban closed 3 years ago

HMarzban commented 3 years ago

Relative issues: #4145, #4207

All Front-end and Back-end test results are the same as the master branch.

HMarzban commented 3 years ago

Nested pad with breadcrumbs demo:

nested-pad

JohnMcLear commented 3 years ago

I have a feeling this will break a lot of things. Are you running frontend tests on pads and nested pads?

I see zero test coverage. You will need a LOT of test coverage for something like this to get merged! Every existing method will need to also be tested in nested etc.

HMarzban commented 3 years ago

I have a feeling this will break a lot of things. Are you running frontend tests on pads and nested pads?

I see zero test coverage. You will need a LOT of test coverage for something like this to get merged! Every existing method will need to also be tested in nested etc.

Well, let me run those tests again, then I will report the results.

JohnMcLear commented 3 years ago

You will need to write tests too. This is a new feature so it will need test coverage.

HMarzban commented 3 years ago 
Etherpad: 1.8.5
Node: v12.16.3
Npm: 6.14.4
Plugins:
    ep_align
    ep_authornames
    ep_comments_page
    ep_ether-o-meter
    ep_headings2
    ep_mammoth
    ep_set_title_on_pad
    ep_webrtc

Note: "Raw" means Etherpad is installed without any plugins. (clone from master)

Raw Test

Front-end Result

Back-end Result

130 passing
2 pending
2 failing

1) Imports and Exports
    exports Etherpad:
    Uncaught Error: Etherpad Document does not include hello
    at Request._callback (C:\Users\---\Desktop\EtherpadTest\RawTest\tests\backend\specs\api\importexportGetPost.js:290:15)
    at Request.self.callback (node_modules\request\request.js:185:22)
    at Request.<anonymous> (node_modules\request\request.js:1154:10)
    at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
    at endReadableNT (_stream_readable.js:1187:12)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)

2) Imports and Exports
    exports HTML for this Etherpad file:
    Uncaught Error: Exported HTML nested list items is not right
    at Request._callback (C:\Users\---\Desktop\EtherpadTest\RawTest\tests\backend\specs\api\importexportGetPost.js:305:15)
    at Request.self.callback (node_modules\request\request.js:185:22)
    at Request.<anonymous> (node_modules\request\request.js:1154:10)
    at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
    at endReadableNT (_stream_readable.js:1187:12)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)

------------------|---------|----------|---------|---------|----------------------------------------------------------------------
File              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------|---------|----------|---------|---------|----------------------------------------------------------------------
 All files        |    59.8 |     34.9 |   43.48 |   59.67 |
 AbsolutePaths.js |   75.61 |    63.16 |     100 |   75.61 | 45-47,56-57,100,104-105,115-116,131
 Cli.js           |   71.43 |       60 |     100 |   69.23 | 34,39,44,49
 Settings.js      |   59.03 |    29.82 |   35.71 |   59.03 | ...700,708-711,715-717,735-769,777-785,792-800,811-816,831
 TidyHtml.js      |      20 |        0 |       0 |      20 | 10-39
 randomstring.js  |     100 |      100 |     100 |     100 |
------------------|---------|----------|---------|---------|----------------------------------------------------------------------

Raw Test with plugins

Back-end Result

  130 passing (7s)
  2 pending
  2 failing

  1) Imports and Exports
       exports Etherpad:
     Uncaught Error: Etherpad Document does not include hello
      at Request._callback (C:\Users\---\Desktop\EtherpadTest\RawTestWithPlugin\tests\backend\specs\api\importexportGetPost.js:290:15)
      at Request.self.callback (node_modules\request\request.js:185:22)
      at Request.<anonymous> (node_modules\request\request.js:1154:10)
      at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
      at endReadableNT (_stream_readable.js:1187:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

  2) Imports and Exports
       exports HTML for this Etherpad file:
     Uncaught Error: Exported HTML nested list items is not right
      at Request._callback (C:\Users\---\Desktop\EtherpadTest\RawTestWithPlugin\tests\backend\specs\api\importexportGetPost.js:305:15)
      at Request.self.callback (node_modules\request\request.js:185:22)
      at Request.<anonymous> (node_modules\request\request.js:1154:10)
      at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
      at endReadableNT (_stream_readable.js:1187:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

------------------|---------|----------|---------|---------|----------------------------------------------------------------------
File              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------|---------|----------|---------|---------|----------------------------------------------------------------------
 All files        |   61.44 |    41.61 |   47.83 |   61.31 |
 AbsolutePaths.js |   75.61 |    63.16 |     100 |   75.61 | 45-47,56-57,100,104-105,115-116,131
 Cli.js           |   71.43 |       60 |     100 |   69.23 | 34,39,44,49
 Settings.js      |   61.23 |     38.6 |   42.86 |   61.23 | ...738,742-744,752-754,758-760,777-785,792-800,811-816,831
 TidyHtml.js      |      20 |        0 |       0 |      20 | 10-39
 randomstring.js  |     100 |      100 |     100 |     100 |
------------------|---------|----------|---------|---------|----------------------------------------------------------------------

Front-end Result

Failures List:

1. ep_comments_page - Comment copy and paste
   - when user copies a text with a comment
      - [e1] "before all" hook for "keeps the text copied on the buffer"
   - when user pastes a text with comment
      - [e2] "before all" hook for "generates a different comment id for the comment pasted"

2. ep_comments_page - Comment Edit
   - and writes a new comment text
     - and presses save
        - [e3] shows the comment text updated
   - when user presses the button edit on a comment reply
      - [e4] should show the edit form
      - [e5] should show the original comment reply text on the edit form
   - and user writes a new comment reply text and presses save
      - [e6] should update the comment text
   - and reloads the page
      - [e7] should update the comment text

3. ep_comments_page - Comment icons
    - [e8] "before each" hook for "hides comment when user clicks on comment icon twice"

4. ep_comments_page - Comment Reply
    - [e9] "before each" hook for "Ensures a comment can be replied"

5. ep_comments_page - Comment Suggestion
    - [e10] "before each" hook for "Fills suggestion Change From field when adding a comment with suggestion"

6. ep_comments_page - Comment Localization
    - [e11] "before each" hook for "uses default values when language was not localized yet"

7. ep_comments_page - Comment settings
   - when user unchecks 'Show Comments'
      - [e12] "before all" hook for "sidebar comments should not be visible when opening a new pad"

8. ep_comments_page - Pre-comment text mark
   - [e13] "before each" hook for "marks selected text when New Comment form is opened"

9. ep_comments_page - Time Formatting
   - in English
      - [e14] "before all" hook for "returns '12 seconds ago' when time is 12 seconds in the past"

10. Set Title On Pad
     - [e15] Checked Default Pad Title is 'Untitled Pad'
     - [e16] Check updating pad title to 'JohnMcLear' works

Raw Test with nested pad support

Front-end Result

Back-end Result

124 passing (21s)
  2 pending
  2 failing

  1) Imports and Exports
       exports Etherpad:
     Uncaught Error: Etherpad Document does not include hello
      at Request._callback (C:\Users\---\Desktop\EtherpadTest\NestedTest\tests\backend\specs\api\importexportGetPost.js:290:15)      at Request.self.callback (node_modules\request\request.js:185:22)
      at Request.<anonymous> (node_modules\request\request.js:1154:10)
      at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
      at endReadableNT (_stream_readable.js:1187:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

  2) Imports and Exports
       exports HTML for this Etherpad file:
     Uncaught Error: Exported HTML nested list items is not right
      at Request._callback (C:\Users\---\Desktop\EtherpadTest\NestedTest\tests\backend\specs\api\importexportGetPost.js:305:15)      at Request.self.callback (node_modules\request\request.js:185:22)
      at Request.<anonymous> (node_modules\request\request.js:1154:10)
      at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
      at endReadableNT (_stream_readable.js:1187:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

------------------|---------|----------|---------|---------|-----------------------------------------------------------------------
File              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------|---------|----------|---------|---------|-----------------------------------------------------------------------
 All files        |   61.44 |    41.61 |   47.83 |   61.31 |
 AbsolutePaths.js |   75.61 |    63.16 |     100 |   75.61 | 45-47,56-57,100,104-105,115-116,131
 Cli.js           |   71.43 |       60 |     100 |   69.23 | 34,39,44,49
 Settings.js      |   61.23 |     38.6 |   42.86 |   61.23 | ...15-717,736-738,742-744,752-754,758-760,777-785,792-800,811-816,831
 TidyHtml.js      |      20 |        0 |       0 |      20 | 10-39
 randomstring.js  |     100 |      100 |     100 |     100 |
------------------|---------|----------|---------|---------|----------------------------------------------------------------------

Raw Test with plugins and nested pad support

Front-end Result

Failures List:

1. ep_comments_page - Comment copy and paste
   - when user copies a text with a comment
      - [e1] "before all" hook for "keeps the text copied on the buffer"
   - when user pastes a text with comment
      - [e1] "before all" hook for "generates a different comment id for the comment pasted"

2. ep_comments_page - Comment Edit
   - when user presses the button edit on a comment
      - and writes a new comment text
         - and reloads the page
            - [e3] shows the comment text updated 
   - when user presses the button edit on a comment reply
      - [e4] should show the edit form
      - [e5] should show the original comment reply text on the edit form
   - and user writes a new comment reply text
      - [e6] and presses save
      - [e7] should update the comment text

3. ep_comments_page - Comment
   - Localization
      - [e8] localizes comment when Etherpad language is changed
      - [e9] localizes 'new comment' form when Etherpad language is changed

4. ep_comments_page - Comment settings
   - when user unchecks 'Show Comments'
      - [e10] sidebar comments should not be visible when opening a new pad
      - [e11] sidebar comments should not be visible when adding a new comment to a new pad ‣

5. ep_comments_page - Time Formatting
   - in Portuguese
      - [e12] returns '12 segundos atrás' when time is 12 seconds in the past
      - [e13] returns 'daqui a 12 segundos' when time is 12 seconds in the future
      - [e14] returns '1 minuto atrás' when time is 75 seconds in the past
      - [e15] returns 'daqui a 1 minuto' when time is 75 seconds in the future
      - [e16] returns '17 minutos atrás' when time is some seconds before 17 minutes in the past
      - [e17] returns 'daqui a 17 minutos' when time is some seconds after 17 minutes in the future
      - [e18] returns '1 hora atrás' when time is some seconds before 1 hour in the past
      - [e19] returns 'daqui a 1 hora' when time is some seconds after 1 hour in the future
      - [e20] returns '2 horas atrás' when time is some seconds before 2 hours in the past
      - [e21] returns 'daqui a 2 horas' when time is some seconds after 2 hours in the future
      - [e22] returns 'ontem' when time is some seconds before 24 hours in the past
      - [e23] returns 'amanhã' when time is some seconds after 24 hours in the future
      - [e24] returns '6 dias atrás' when time is some seconds before 6 days in the past
      - [e25] returns 'daqui a 6 dias' when time is some seconds after 6 days in the future
      - [e26] returns 'semana passada' when time is some seconds before 7 days in the past
      - [e27] returns 'próxima semana' when time is some seconds after 7 days in the future
      - [e28] returns '2 semanas atrás' when time is some seconds before 2 weeks in the past
      - [e29] returns 'daqui a 2 semanas' when time is some seconds after 2 weeks in the future
      - [e30] returns 'mês passado' when time is some seconds before 4 weeks in the past
      - [e31] returns 'próximo mês' when time is some seconds after 4 weeks in the future
      - [e32] returns '9 meses atrás' when time is some seconds before 9 months in the past
      - [e33] returns 'daqui a 9 meses' when time is some seconds after 9 months in the future
      - [e34] returns 'ano passado' when time is some seconds before 12 months in the past
      - [e35] returns 'próximo ano' when time is some seconds after 12 months in the future
      - [e36] returns '15 anos atrás' when time is some seconds before 15 years in the past
      - [e37] returns 'daqui a 15 anos' when time is some seconds after 15 years in the future
      - [e38] returns 'século passado' when time is some seconds before 100 years in the past
      - [e39] returns 'próximo século' when time is some seconds after 100 years in the future
      - [e40] returns '2 séculos atrás' when time is some seconds before 2 centuries in the past
      - [e41] returns 'daqui a 2 séculos' when time is some seconds after 2 centuries in the future

Back-end Result

  124 passing (32s)
  2 pending
  2 failing

  1) Imports and Exports
       exports Etherpad:
     Uncaught Error: Etherpad Document does not include hello
      at Request._callback (C:\Users\---\Desktop\EtherpadTest\NestedTestWithPlugin\tests\backend\specs\api\importexportGetPost.js:290:15)
      at Request.self.callback (node_modules\request\request.js:185:22)
      at Request.<anonymous> (node_modules\request\request.js:1154:10)
      at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
      at endReadableNT (_stream_readable.js:1187:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

  2) Imports and Exports
       exports HTML for this Etherpad file:
     Uncaught Error: Exported HTML nested list items is not right
      at Request._callback (C:\Users\---\Desktop\EtherpadTest\NestedTestWithPlugin\tests\backend\specs\api\importexportGetPost.js:305:15)
      at Request.self.callback (node_modules\request\request.js:185:22)
      at Request.<anonymous> (node_modules\request\request.js:1154:10)
      at IncomingMessage.<anonymous> (node_modules\request\request.js:1076:12)
      at endReadableNT (_stream_readable.js:1187:12)
      at processTicksAndRejections (internal/process/task_queues.js:84:21)

------------------|---------|----------|---------|---------|----------------------------------------------------------------------
File              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------|---------|----------|---------|---------|----------------------------------------------------------------------
 All files        |   61.44 |    41.61 |   47.83 |   61.31 |
 AbsolutePaths.js |   75.61 |    63.16 |     100 |   75.61 | 45-47,56-57,100,104-105,115-116,131
 Cli.js           |   71.43 |       60 |     100 |   69.23 | 34,39,44,49
 Settings.js      |   61.23 |     38.6 |   42.86 |   61.23 | ...5-717,736-738,742-744,752-754,758-760,777-785,792-800,811-816,831
 TidyHtml.js      |      20 |        0 |       0 |      20 | 10-39
 randomstring.js  |     100 |      100 |     100 |     100 |
------------------|---------|----------|---------|---------|----------------------------------------------------------------------
HMarzban commented 3 years ago

You will need to write tests too. This is a new feature so it will need test coverage.

Sure, I'm writing new tests for nested pads.

JohnMcLear commented 3 years ago

Test with out plugins please :+1:

Certain plugins are automatically tested when we merge.

HMarzban commented 3 years ago

@JohnMcLear Well, I added a new test router for the nested pad for a full test and named it: /tests/nested-pad/frontend. I also wrote a simple test case for nested pad Ids.

/tests/frontend

/tests/nested-pad/frontend

JohnMcLear commented 3 years ago

Good start, have left comments, I'd say you are 20% or so through getting this merged in it's current state. It's all fixable I just think the internals need documenting better and more test coverage.

HMarzban commented 3 years ago

What i've done for this new push

  1. Favicon static path and URL path logic move to settings controller
  2. The new router that i created in the last push was removed(/tests/nested-pad/frontend, also the querystring remove too), now on when the helper.newPad() function is called, the nested pad is created randomly.
  3. Regarding proxy concerns, I checked all the configurations in the document, no need to change, everything are works well.
  4. From now on padId, staticRootAddress and padName(A new property that is created and sent from routers; not from client-side and pick up from url path) which are avilable in clientVars.

Note: All front-end tests were successful

Explanation for Pad Id and Pad Name

The userId and padName of this link /p/pad1/pad2/pad3 should be like this:

const padId = "pad1:pad2:pad3"
const padName = "pad3"

Our only concern with padId is the limitations we have on the number of characters for the key ID.

What's left and we have to talk about it

Questions

  1. Are you happy with this padId naming? (redis key naming convention)
  2. We could seperate padId, padName and make slug, do you have any idea for that?
  3. The export and import routers are depend on /p/:pad* and it's difficult to adapt to the nested pad, would you want to change this rule, and then create a separate router for each of them? (/export/:pad* and /import/:pad*, looks more standard)
JohnMcLear commented 3 years ago
  1. Well the padId naming may break ueberDB as it's the convention also used for that, but without testing IDK. You need test coverage in to continue this conversation. For reference see ueberDB findKeys
  2. No idea RE slug.
  3. The suffixed endpoints for import/export can't change, they will be hardcoded into peoples logic IE $padID+/export/pdf -- you will need to find a way to support $padID+"/"+$padID2+"/export/...."
HMarzban commented 3 years ago

@JohnMcLear Well, well, I finally wrote all the test cases that cover the changes I made for the nested pad feature. Could you take a look and review that?

rhansen commented 3 years ago

What happens if a user creates a nested pad named import or export? Paths like /p/foo/import and /p/foo/export conflict with the import and export handlers. Are there tests for these cases?

HMarzban commented 3 years ago

Yes, I tested the "Import" and "Export" features, also wrote the new test for that, just run the front-end test and see the results.

JohnMcLear commented 3 years ago

Please resolve conflicts.

rhansen commented 3 years ago

Heads up: webaccess.js line 67 was recently changed and will probably need tweaking for this PR.

rhansen commented 3 years ago

I'm not a fan of using / as the pad hierarchy separator in the URL because it conflicts with paths like /p/${padId}/import, /p/${padId}/export, and /p/${padId}/timeslider. It also runs the risk of violating assumptions made in plugins or in the dark, untested corners of Etherpad's code. I'd prefer some other character, such as colon (%3A) or vertical line (%7C). Or, to avoid breaking compatibility with existing pad IDs, perhaps the hierarchy separator should be an exotic character such as U+001F (%1F). Using a percent-encoded slash (%2F) would also work, but that might be confusing.

HMarzban commented 3 years ago

@rhansen, I get all new changes to my fork, unfortunately, i got an error! It tells, The module at "js-cookie/src/js.cookie" does not exist, i can not resolve this issue, because there is no file name js.cookie in the project directory.

Could you please help me?

JohnMcLear commented 3 years ago

@rhansen, I get all new changes to my fork, unfortunately, i got an error! It tells, The module at "js-cookie/src/js.cookie" does not exist, i can not resolve this issue, because there is no file name js.cookie in the project directory.

Could you please help me?

This caught me out too.. you need to update your node deps..

cd src && npm install

or bin/run.sh

HMarzban commented 3 years ago

I'm not a fan of using / as the pad hierarchy separator in the URL because it conflicts with paths like /p/${padId}/import, /p/${padId}/export, and /p/${padId}/timeslider, and it runs the risk of violating assumptions made in plugins or in the dark, untested corners of Etherpad's code. I'd prefer some other character, such as colon (%3A) or vertical line (%7C). Or, to avoid breaking compatibility with existing pad IDs, perhaps the hierarchy separator should be an exotic character such as U+001F (%1F). Using a percent-encoded slash (%2F) would also work, but that might be confusing.

You right, this doesn't appear to be the correct pattern for the pad hierarchy, therefor I asked @JohnMcLear to change the pattern, but unlikely, it will breaks all old versions and backward compatibility. (so I changed the router to make this import and export pad hierarchy work, without breaking anything.)

sonarcloud[bot] commented 3 years ago

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 2 Security Hotspots to review)
Code Smell A 9 Code Smells

No Coverage information No Coverage information
5.9% 5.9% Duplication

JohnMcLear commented 3 years ago

You right, this doesn't appear to be the correct pattern for the pad hierarchy, therefor I asked @JohnMcLear to change the pattern, but unlikely, it will breaks all old versions and backward compatibility. (so I changed the router to make this import and export pad hierarchy work, without breaking anything.)

I don't care about the pattern. The point is that getSub of UeberDB already uses foo:bar:nemo schema.

See for example

./db/Pad.js:140:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "timestamp"]);
./db/Pad.js:144:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["changeset"]);
./db/Pad.js:148:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "author"]);
./db/Pad.js:152:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "timestamp"]);
./db/Pad.js:179:  let p_atext = db.getSub("pad:" + this.id + ":revs:" + keyRev, ["meta", "atext"]);
./db/AuthorManager.js:149:  return db.getSub("globalAuthor:" + author, ["colorId"]);
./db/AuthorManager.js:168:  return db.getSub("globalAuthor:" + author, ["name"]);
./db/GroupManager.js:179:  let result = await db.getSub("group:" + groupID, ["pads"]);
./db/DB.js:51:      ['get', 'set', 'findKeys', 'getSub', 'setSub', 'remove', 'doShutdown'].forEach(fn => {
./db/DB.js:55:      // set up wrappers for get and getSub that can't return "undefined"
./db/DB.js:62:      let getSub = exports.getSub;
./db/DB.js:63:      exports.getSub = async function(key, sub) {
./db/DB.js:64:        let result = await getSub(key, sub);
HMarzban commented 3 years ago

You right, this doesn't appear to be the correct pattern for the pad hierarchy, therefor I asked @JohnMcLear to change the pattern, but unlikely, it will breaks all old versions and backward compatibility. (so I changed the router to make this import and export pad hierarchy work, without breaking anything.)

I don't care about the pattern. The point is that getSub of UeberDB already uses foo:bar:nemo schema.

See for example

./db/Pad.js:140:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "timestamp"]);
./db/Pad.js:144:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["changeset"]);
./db/Pad.js:148:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "author"]);
./db/Pad.js:152:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "timestamp"]);
./db/Pad.js:179:  let p_atext = db.getSub("pad:" + this.id + ":revs:" + keyRev, ["meta", "atext"]);
./db/AuthorManager.js:149:  return db.getSub("globalAuthor:" + author, ["colorId"]);
./db/AuthorManager.js:168:  return db.getSub("globalAuthor:" + author, ["name"]);
./db/GroupManager.js:179:  let result = await db.getSub("group:" + groupID, ["pads"]);
./db/DB.js:51:      ['get', 'set', 'findKeys', 'getSub', 'setSub', 'remove', 'doShutdown'].forEach(fn => {
./db/DB.js:55:      // set up wrappers for get and getSub that can't return "undefined"
./db/DB.js:62:      let getSub = exports.getSub;
./db/DB.js:63:      exports.getSub = async function(key, sub) {
./db/DB.js:64:        let result = await getSub(key, sub);

Yeah, I saw it, and I added test cases to it in the nestedPad test coverage.(passes without any failures)

JohnMcLear commented 3 years ago

You right, this doesn't appear to be the correct pattern for the pad hierarchy, therefor I asked @JohnMcLear to change the pattern, but unlikely, it will breaks all old versions and backward compatibility. (so I changed the router to make this import and export pad hierarchy work, without breaking anything.)

I don't care about the pattern. The point is that getSub of UeberDB already uses foo:bar:nemo schema. See for example

./db/Pad.js:140:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "timestamp"]);
./db/Pad.js:144:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["changeset"]);
./db/Pad.js:148:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "author"]);
./db/Pad.js:152:  return db.getSub("pad:" + this.id + ":revs:" + revNum, ["meta", "timestamp"]);
./db/Pad.js:179:  let p_atext = db.getSub("pad:" + this.id + ":revs:" + keyRev, ["meta", "atext"]);
./db/AuthorManager.js:149:  return db.getSub("globalAuthor:" + author, ["colorId"]);
./db/AuthorManager.js:168:  return db.getSub("globalAuthor:" + author, ["name"]);
./db/GroupManager.js:179:  let result = await db.getSub("group:" + groupID, ["pads"]);
./db/DB.js:51:      ['get', 'set', 'findKeys', 'getSub', 'setSub', 'remove', 'doShutdown'].forEach(fn => {
./db/DB.js:55:      // set up wrappers for get and getSub that can't return "undefined"
./db/DB.js:62:      let getSub = exports.getSub;
./db/DB.js:63:      exports.getSub = async function(key, sub) {
./db/DB.js:64:        let result = await getSub(key, sub);

Yeah, I saw it, and I added test cases to it in the nestedPad test coverage.(passes without any failures)

While we might have what we think is good test coverage making such a change could break plugins / other peoples expectation for pad URIs and also if people are using set/getSub on foo:bar where they want bar it will fail if it's a nested pad. For that reason I suggest we put nesting under a setting, that would mitigate transitional risk.

I'm also going to require that someone other than the developer (@HMarzban) is tasked w/ writing test coverage and finding edge cases. I suggest hiring @webzwo0i for that task. My logic and reasoning here is 1) If @HMarzban doesn't provide ongoing support for this feature (I would require at least 12 months at a minimum) and it's not fair on only @HMarzban to be the only one truly familiar and carry the burden of the feature. It's also reallllly easy to "miss the forest for the trees" and having fresh eyes could and most likely will uncover issues that a developer cannot see because they are so heavily involved in the nuance of the feature.

HMarzban commented 3 years ago

Good move, thanks.

HMarzban commented 3 years ago

@JohnMcLear do you think I should write more tests and make this feature available in global settings? (make this feature optional)

JohnMcLear commented 3 years ago

Yes and talk to @webzwo0i to get his pricing and availability. I will want at least one full day of his time reviewing this and letting me know the technical debt of the implementation.

JohnMcLear commented 3 years ago

@HMarzban what's the status? I'm worried this will go stale really quickly if we don't get it merged.

edsaperia commented 3 years ago

I am waiting for a response from @webzwo0i.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

JohnMcLear commented 3 years ago

Closing as stale.