CSL-2041: Popover tours #336

mbrambilla · 2022-10-28T20:02:02Z

Re-implementation of PR #333.

As noted previously, there are updates to the tips_tiptype that are needed. Please refer to notes from @klown in PR # regarding [tiptypes.json](https://github.com/cast-org/clusive/blob/0e70bc499b1566f8fe00afe77ff1ef2330b4517d/src/tips/fixtures/tiptypes.json) and issues with updating the db.

I am are there is a way to get a migration file, as this is also possibly needed for PR #328. ~~Potential solution: https://stackoverflow.com/questions/4002322/migrating-django-fixtures~~ Actually this appears to be more of a create fixture, not a migration.

Reference: https://code.djangoproject.com/ticket/24778

klown · 2022-10-28T20:32:44Z

I don't believe any migration is required for PR #328 because the database structure for the TipTypes remains the same. That is, no new columns are needed.

What was needed was to add a number of records for the new types. The last priority value before adding the new set was 8 for the activity tip. I added the new tip types starting with priority 9 for student_reactions, increasing by 1 for every new tip. Running loaddata gave no errors.

However, when later removing the cluey tip, it created a hole and the priorities needed to shift. Running loadata thereafter resulted in uniqueness errors. I solved that by hand: Using the administrator's interface to the database, I removed all of the tip types and all related Events. It wasn't too bad since it's a development site and I often clear Events out anyway, just to save space. Thereafter, I could load the new tip types and their latest priorities from the json fixture into an empty TipType table.

But that approach is not feasible for Q/A nor Production since there are a lot of Event records there, and I assume we want to keep them, and continue to have them reference the same tip types as they do now.

I'll look at the stack overflow article and also search for any documents regarding migration and unique fields. Another possibility is to write a python script that:

Get the tip with the largest priority,
Increase its priority by one and save it,
Repeat until all current tips have a new priority one greater than before.

That should create a hole at the top that allows loaddata to run without a uniqueness error and add a tour tip type with priority 1.

klown · 2022-10-28T22:13:14Z

@mbrambilla, I put together a quick and dirty incrementpriorities that does what I outlined above to increment each tip type's priority by one, starting with the tip that has the highest priority. It works and creates a "hole" in that no tip type has a priority of 1 any longer. Running loaddata thereafter where the tiptypes.json has a tour type with a priority value equal to 1 succeeds.

But, I noticed that all of the tip type's primary keys initially matched their priority. After running the increment manage command, they no longer match. I don't know if that's important. Also, I'm almost certain that the primary keys cannot be changed since that is what the database actually uses to associate the tip type with other records like Events, or TipHistorys. In addition, the json fixture file defines the primary key with the 'pk' field for each type. The new tour tip had to have a 'pk': 16 to make sure its primary key didn't collide with any other type.

I have to ponder this some more.

klown · 2022-10-28T22:33:41Z

@mbrambilla If you want to check out the script, I've created a draft branch and PR based on this branch/PR in PR #337.

Use with caution :-)

bgoldowsky · 2022-10-31T18:01:22Z

The priority can certainly be different from the pk - that they matched is just historical accident. Pk's should not be changed to avoid breaking DB connections, but priorities can be changed as needed. As far as I recall, the actual value of priority also doesn't matter - just the ordering. So holes are fine.

klown · 2022-10-31T19:23:55Z

Thanks for the clarifications @bgoldowsky !

In that case, is the incrementpriorities manage command good enough? It creates a hole in terms of priorities such that the tour tip can be inserted. That command is part of PR #337, as well as a version of tiptypes.json that includes the tour tip with a priority of 1.

# Conflicts: # src/library/views.py # src/pages/views.py # src/roster/views.py # src/shared/templates/shared/partial/popover_tour.html # src/shared/templates/shared/partial/popover_tour_footer.html

avoids the need for lots of other priority changes.

Logic was not in sync with can_show; use that method so that it is consistent. Make it a class method with standard capitalization convention. Note this changes the order to use priority; should fix that.

This avoids having to depend on ordering in the code.

bgoldowsky · 2022-11-16T20:56:38Z

@mbrambilla @klown I made some changes, would love to get your feedback on them.

There is one remaining issue; there's at least one case where a tip is legal but just can't attach to anything (when a brand new user doesn't have a react wheel on their dashboard yet because they haven't read a book). This type of case is handled reasonably gracefully for single tips (that's why we added the 'tip viewed' message sent back from the client side), but breaks the tour with a JS error. We either need to (a) put some defensive code in the tour JS that will skip over tips that are not actually anchored to the page, or (b) add more conditions to TipType.can_show() to try to catch this and any other case where a tooltip won't be able to be shown. WDYT?

mbrambilla · 2022-11-16T21:35:08Z

Some checking is possible in the tour.chain() JS side, but then the tour list would need to be passed to the JS and the popover footers would need to be dynamically updated if an item is missing or hidden.

The current implementation has the inter-linking of the tour popovers is handled through the template code through the loop iteration (helped by the list_index_prev and list_index_next filters) so we didn't have to do all the chaining, counting, and HTML manipulation through JS.

We might also need to handle the possible case of items being shown/hidden based on the tab interface of some of the panels (I never double-checked).

However, if it is this one case, would it might be easier (faster) to remove the missing tour item from the list on the Django side based on a conditional to see if the student has not read any books.

klown · 2022-11-16T21:39:27Z

src/tips/fixtures/tiptypes.json

    "fields": {
      "name": "tour",
-      "priority": 1,
+      "priority": 0,


Very nice. And, it makes PR #337 to shuffle the priorities around unnecessary*.

<curmudgeon>And here I thought that zero was neither positive nor negative. Django's PositiveSmallIntegerField says otherwise</curmudgeon>

* for now...

One of those areas where math ≠ computer science, I guess?

bgoldowsky · 2022-11-16T22:22:00Z

Why is 'thoughts' classified as student only? Any reason not to include it in the teacher tour of the reading page?

bgoldowsky · 2022-11-16T22:22:51Z

BTW I am working on revising can_show() along the lines of Matt's suggestion.

klown · 2022-11-16T22:23:27Z

I'm not for changing TipType.can_show() since it's already relatively complicated. However, @mbrambilla 's outline of how complicated it is to handle missing tour popovers in the JS suggests the can_show() is the lesser of two evils.

Regarding the missing popover "breaks the tour with a JS error": what is the nature of the error? Is it an exception that could be caught and somehow skipped over? Or some generic error?

bgoldowsky · 2022-11-16T22:26:23Z

It's a NPE because it tries to activate a popover in the chain but it's not there. The error could be caught, but would be awkward at best - the number of steps in the tour shown in the footer would be wrong, 'next' or 'prev' buttons would be active when they shouldn't be, etc. I think Matt is right that we need to make the Django side accurate as to what items are going to be available. I think I can make it not too ugly.

klown · 2022-11-16T22:29:53Z

Why is 'thoughts' classified as student only? Any reason not to include it in the teacher tour of the reading page?

That's a question for Kristin/Kim/Agatha. Kristin listed it as such in CSL-2040, and the content (google doc) also shows it as "student only" under the "Link to video [teacher]" heading: "[student only tip]".

bgoldowsky · 2022-11-16T22:52:53Z

I think that was probably a misunderstanding since there was considerable confusion between 'thoughts' and 'student_reactions'. I can check with Kristin.

klown · 2022-11-16T23:01:04Z

More re: "thoughts" and it being student only. I think there's a bug here. The video for student-only "thoughts" is actually for the student_reactions tip on the student dashboard. There was a time when that tip for the student dashboard was called "thoughts" which raised other problems. I switched it to student_reactions for the student dashboard, but I may not have updated can_show() to reflect that. I'm still investigating and I believe that looking at the popover's contents for the "Your thoughts" widget on the reading page would help to decide, but I'm having some difficulty finding it.

klown · 2022-11-16T23:19:37Z

I found it and it is aimed at students. Part of the text for the popover notes that all of your reactions can be found on the student dashboard. The video is somewhat less student-centric, but at one point says, "... your teacher ..." which implies the user is a student, and also points out the reaction wheel on the student dashboard. Still the beginning of the video is fairly generic in describing the dialog for filling in reactions, answering the custom question, and so on.

Here's the link: https://docs.google.com/document/d/1SYru98tt_Zmzz2z_7lKDCSiKqsOdDFTTLp0XMvW97h0/edit
Note that the document says "Student only tip" here as well.

So, I take back that I think there is bug in can_show() :-)

Clean up a few other small issues in tour.

bgoldowsky · 2022-11-17T18:59:12Z

Thanks @klown for the investigation. I acknowledge that Kristin has specified 'thoughts' as student-only. Nevertheless, I am going to go with my gut feeling for now that there is no reason for it to be the single outlier student-only tooltip, until I can actually have a conversation with Kristin about it.

CSL-2041: Popover tours

0e70bc4

CSL-2041: Add view tracking for popover when viewed in chain

8c059af

mbrambilla mentioned this pull request Nov 1, 2022

Feature/CSL-2007/CSL-2040 group 1 and 2 enhanced tooltips #328

Merged

Merge branch 'development' into feature/CSL-2041-popover-tour-2

d2f343f

# Conflicts: # src/library/views.py # src/pages/views.py # src/roster/views.py # src/shared/templates/shared/partial/popover_tour.html # src/shared/templates/shared/partial/popover_tour_footer.html

mbrambilla changed the base branch from feature/CSL-2040-group-2-enhanced-tooltips to development November 16, 2022 14:41

bgoldowsky added 3 commits November 16, 2022 11:28

CSL-2041: Adding "tour" tip as priority 0

7a05136

avoids the need for lots of other priority changes.

CSL-2041: fix TourList

0bff58a

Logic was not in sync with can_show; use that method so that it is consistent. Make it a class method with standard capitalization convention. Note this changes the order to use priority; should fix that.

CSL-2041: Make tour position an explicit property of TipType.

1e37463

This avoids having to depend on ordering in the code.

klown reviewed Nov 16, 2022

View reviewed changes

CSL-2041: Improve can_show()

e9233c0

Clean up a few other small issues in tour.

bgoldowsky merged commit df41457 into development Nov 17, 2022

CSL-2041: Popover tours #336

CSL-2041: Popover tours #336

Uh oh!

Conversation

mbrambilla commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klown commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klown commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klown commented Oct 28, 2022

Uh oh!

bgoldowsky commented Oct 31, 2022

Uh oh!

klown commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgoldowsky commented Nov 16, 2022

Uh oh!

mbrambilla commented Nov 16, 2022

Uh oh!

klown Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

bgoldowsky Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

bgoldowsky commented Nov 16, 2022

Uh oh!

bgoldowsky commented Nov 16, 2022

Uh oh!

klown commented Nov 16, 2022

Uh oh!

bgoldowsky commented Nov 16, 2022

Uh oh!

klown commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgoldowsky commented Nov 16, 2022

Uh oh!

klown commented Nov 16, 2022

Uh oh!

klown commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bgoldowsky commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mbrambilla commented Oct 28, 2022 •

edited

Loading

klown commented Oct 28, 2022 •

edited

Loading

klown commented Oct 28, 2022 •

edited

Loading

klown commented Oct 31, 2022 •

edited

Loading

klown commented Nov 16, 2022 •

edited

Loading

klown commented Nov 16, 2022 •

edited

Loading

bgoldowsky commented Nov 17, 2022 •

edited

Loading