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

Notebook Snapshot menu is only updating section and page names for the editor's view #3982

Closed
charlesh88 opened this issue Jun 28, 2021 · 10 comments · Fixed by #4002
Closed

Comments

@charlesh88
Copy link
Contributor

charlesh88 commented Jun 28, 2021

Tiffany and I were interacting with the same Notebook section and page to discuss multi-user Notebook user tests, and while she did see updated names for the section and page in the Snapshot menu, I did not:

Screen Shot 2021-06-28 at 11 23 27 AM

This condition remained even after clearing cache and refreshing.

It appears as though updating is based on who did the naming. In the example above, she had created and named both "Flight Day 1" and "Shift 1". In this second example, I added a new page to Flight Day 1 and named it "Shift 2". My Snapshot menu shows the correct name for the page that I named, but shows "Unnamed section" for the section that Tiffany edited:

Screen Shot 2021-06-28 at 11 56 43 AM

Detail Notebook testing DOC:

https://docs.google.com/document/d/1Y_95OumOH5JZ2j079tL50orTBSExJ9o8yqXhGrPLM1A/edit#

@charlesh88 charlesh88 changed the title Notebook Snapshot menu is sporadically not updating section and page names Notebook Snapshot menu is only updating section and page names for the editor's view Jun 28, 2021
@akhenry
Copy link
Contributor

akhenry commented Jun 28, 2021

This was picked up in an earlier PR, but it appears that it was never actually fixed, presumably due to a misunderstanding. As I mentioned in that PR, the correct approach is to only store the object identifier and resolve the object dynamically. Do not store any attributes of an object other than the identifier in local storage, because they are guaranteed to go stale.

@akhenry akhenry assigned nikhilmandlik and unassigned akhenry Jun 28, 2021
@nikhilmandlik
Copy link
Contributor

yes I think we made that change long time back, let me check

@unlikelyzero unlikelyzero added this to Needs triage in Bug Tracker via automation Jul 7, 2021
@unlikelyzero
Copy link
Collaborator

Removing CDA Tag per @akhenry

@nikhilmandlik
Copy link
Contributor

following observation after initial analysis,

  1. each notebook user can have own default notebook.
  2. this leads to each user can have their own default section and default page
  3. so cant save default section/page on notebook domainobject.
  4. we need to store following items in localstorage
    defaultNotebook identifier
    defaultNotebook.sectionId
    defaultNotebook.pageId
  5. currently storing section/page but using name directly from it (instead using ids to fetch latest values)

but this leads some edge cases to handle:
1 . 2 different users have same default notebook but different section and pages and one of them deletes default section/page of other user
1 . 2 different users have different default notebook and one of the user deletes default notebook/section/page of other user.

@akhenry
Copy link
Contributor

akhenry commented Jul 8, 2021

Correct, default section and page ID cannot be persisted on the object because it differs by user.

Yeah I had some of the same concerns about those edge cases when I was recently working on Notebooks, and I was wondering whether we really want per-user a default section and default page, or whether we could just take them to the last page updated by anybody.

In any case, the edge cases are easily resolved:

  1. If the default page and section not longer exist, just go to the last page and section (or the most recently edited page in any section)
  2. If a user's default notebook has been deleted just don't do anything. Don't show the option in the snapshot menu, and don't highlight anything in the tree.

@charlesh88 thoughts?

@charlesh88
Copy link
Contributor Author

charlesh88 commented Jul 8, 2021

Correct, default section and page ID cannot be persisted on the object because it differs by user.

Yeah I had some of the same concerns about those edge cases when I was recently working on Notebooks, and I was wondering whether we really want per-user a default section and default page, or whether we could just take them to the last page updated by anybody.

In any case, the edge cases are easily resolved:

  1. If the default page and section not longer exist, just go to the last page and section (or the most recently edited page in any section)
  2. If a user's default notebook has been deleted just don't do anything. Don't show the option in the snapshot menu, and don't highlight anything in the tree.

@charlesh88 thoughts?

I'm actually not a fan of the default Notebook stuff exactly because of the complications that ensue - but we're saddled with it. That said, users absolutely must have their own defaults. The use case here is I come on shift and create a new Notebook entry in a Notebook/section/page. All subsequent entries, snapshots, etc. must go to that page. If someone else logs in and suddenly all my snapshots are going to another unexpected Notebook, that would be very bad.

So as just discussed:

  • Users must have their own defaults
  • Deleting a Notebook, section or page that's currently the default results in that user no longer having a default option on their Snapshot menu. We will address the problem of deletion at large in the application in the future.

Bug Tracker automation moved this from Needs triage to Closed Aug 12, 2021
shefalijoshi added a commit that referenced this issue Aug 12, 2021
…e editor's view #3982 (#4002)

* if default section/page is missing then clear default notebook and hide option from dropdown.

* handle edge case when section is null/undefined.

* refactored notebook localstorage data + fixed some edge cases when default section/page gets deleted.

Co-authored-by: Shefali Joshi <simplyrender@gmail.com>
@unlikelyzero
Copy link
Collaborator

unlikelyzero commented Aug 24, 2021

Will retest after #4144 is fixed

@shefalijoshi
Copy link
Contributor

Verified fixed.

@charlesh88
Copy link
Contributor Author

Testing Notes

This needs two users looking at the same Notebook.

  1. User 1 creates a new Notebook, and in the process enters custom values for Section and Page names.
  2. User 1 sends the link to that Notebook to User 2.
  3. User 2 goes to the link, and should see the updated Section and Page names.
  4. User 2 should make an entry in the Notebook, setting that section and page to the default.
  5. User 1 should make an entry in the renamed section and page, same as User 2.
  6. Both users should verify that the Notebook Snapshot dropdown displays the updated Section and Page names.

@charlesh88
Copy link
Contributor Author

Testathon 8-26-21: verified fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants