Remove tour to drop problematic dependency #5861
Conversation
|
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5861 +/- ##
==========================================
- Coverage 98.51% 98.51% -0.01%
==========================================
Files 395 394 -1
Lines 38782 38730 -52
==========================================
- Hits 38205 38153 -52
Misses 577 577 ☔ View full report in Codecov by Sentry. |
| @@ -31,4 +31,7 @@ $t->ua(OpenQA::Client->new(apikey => 'LANCELOTKEY01', apisecret => 'MANYPEOPLEKN | |||
| $t->app($app); | |||
| $t->delete_ok('/api/v1/user/99904')->status_is(403, 'non-admins cannot delete users'); | |||
|
|
|||
| $t->post_ok('/api/v1/feature?version=42')->status_is(200, 'can set the feature version of current user'); | |||
| is $app->schema->resultset('Users')->find(99902)->feature_version, 42, 'feature version was updated'; | |||
There was a problem hiding this comment.
kinda nitpicking but I think the test checks the value in the DB. it doesnt update it. I mean I understand that it reffers to the previous action but maybe rephrase it.
Something like Schema returns correct feature version or Correct feature version value can be found in database. wdyt?
There was a problem hiding this comment.
It is a post, though? And it checks the new version
There was a problem hiding this comment.
I would not mention the database explicitly here. If we did this in similar cases we would lengthen a lot of lines in our code. Note that the test names are also generally supposed to explain the check on a higher level instead of just repeating what the code does.
I think using the past here makes it clear enough that the updating was done before the check and the check is just verifying what happened before (in this case the post).
| <div class="dropdown-menu"> | ||
| %= tag 'h3' => class => 'dropdown-header' => 'Operators Menu' | ||
| %= link_to 'Activity View' => url_for('activity_view') => class => 'dropdown-item' => id => 'activity_view' | ||
| %= link_to 'Activity View' => url_for('activity_view') => class => 'dropdown-item' => id => 'activity_view', title => 'Gives you an overview of your current jobs' |
There was a problem hiding this comment.
Maybe Provides an overview of your current jobs sounds better? Feel free to resolve this if you think it does not.
There was a problem hiding this comment.
After a quick research I think both verbs are fine and "give an overview" is probably even the more commonly used phrase.
There was a problem hiding this comment.
I think you misunderstood our goals for https://progress.opensuse.org/issues/163004. I am against removing the tour. You can replace it with bootstrap hints as suggested. If I am reading your changes correctly then you are just putting help text in kinda hidden places. The ticket subject says "a simple bootstrap hint pointing to first steps"
|
So I don't misunderstand this again: What do you mean with "boostrap hint" and "first steps"? Note that the tour so far just explained some UI elements that are in my opinion rather self-explanatory - especially with the newly added tooltips. It did not tell one first steps (like what we have in the "Getting started" documentation). The tour was also in general not about first steps but to notify users about new features (except that actually extending the tour was not really feasible in most cases and if we would have done it consequently we would now have a tour that is way to lengthy to click though from the beginning). |
It may break these tests because we probably dismiss the tour at some point. |
There was a problem hiding this comment.
So I don't misunderstand this again: What do you mean with "boostrap hint" and "first steps"?
I actually meant a "bootstrap popover" to point to a single menu entry where newly logged in users probably want to start. But please read on for my reconsideration :)
Note that the tour so far just explained some UI elements that are in my opinion rather self-explanatory - especially with the newly added tooltips. It did not tell one first steps (like what we have in the "Getting started" documentation). The tour was also in general not about first steps but to notify users about new features (except that actually extending the tour was not really feasible in most cases and if we would have done it consequently we would now have a tour that is way to lengthy to click though from the beginning).
Correct. Just from the point of view of a beginner one should be presented with unobstrusive hints where to get started. Previously we had the four steps "All tests area", "Job group menu", "User menu", "Activity View". I like how you moved some text already into help for the entries without needing those to be tour-popups. This shows how we are really liking good "title" attributes in many more places :)
I was thinking about maybe a single popup for new users where to start, like click on "All Tests" for a start. But then I figured that depending on the instance the index page or the job groups might be more likely what new users would look for. So I guess I am ok with your approach.
Just I would prefer to have the git commit message subject and PR title rephrased to be user-facing. How about "Use better help on menu items instead of obtrusive tour". And in the details you could mention the secondary benefit for us maintainers that by this we are dropping the problematic dependency :)
kalikiana
dismissed
their stale review
I guess we don't agree on what to implement here?
* Remove tour dropping problematic dependency `shepherd.js` * Keep the database column to avoid a problematic database migration * Keep the API endpoint to set the tour progress to avoid a breaking API change * Note that we might want to repurpose the database column and API in the future to show a simple help notification for new users * Add a very small test for the feature API so it is still covered after removing the UI test of the feature tour * Remove all UI code related to the tour * See https://progress.opensuse.org/issues/163004 for further reasoning
* Note that this little information is really all the tour provided; it might make sense to add further tooltips in the future * Note that the jumptron already points out the most relevant documentation pages via our brandings * See https://progress.opensuse.org/issues/163004
|
After @okurz 's last comment I hope we have found an agreement. So please re-review my latest changes. |
See particular commit messages and https://progress.opensuse.org/issues/163004
I recommend to look at the individual commits.