-
Notifications
You must be signed in to change notification settings - Fork 204
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
Dynamic feature infos for users #1388
Dynamic feature infos for users #1388
Conversation
lib/OpenQA/Schema/Result/Users.pm
Outdated
data_type => 'text', | ||
is_nullable => 1, | ||
}, | ||
feature_informed => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be feature informed? (as it is a boolean... )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foursixnine It is for checking if the user is already informed about the latest/current features or not. If he saw the newest features the boolean is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so with every new feature you would have to write a migration to reset? that's not going to work - you will need to store exactly what feature she was informed about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coolo i want to do this in this area:
last_login_version => { data_type => 'text', is_nullable => 1, },
the boolean part is just for checking if the user is informed about the newest feature.
So lets say there is feature01, feature02, ...feature05 in the last_login_version (maybe i should change the description). The jquery code will check for the version number, and if the user saw the latest feature (lets say feature05), the boolean part will set to true. Is this the right direction?
lib/OpenQA/Schema/Result/Users.pm
Outdated
data_type => 'text', | ||
is_nullable => 1, | ||
}, | ||
feature_informed => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so with every new feature you would have to write a migration to reset? that's not going to work - you will need to store exactly what feature she was informed about
lib/OpenQA/WebAPI.pm
Outdated
@@ -466,6 +466,10 @@ sub startup { | |||
## JSON API ends here | |||
# | |||
|
|||
# api/v1/feature | |||
$api_public_r->post('/feature')->name('apiv1_post_informed_about')->to('feature#informed'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public does not make sense - you want the user
sub check { | ||
my ($self) = @_; | ||
my $user = $self->current_user; | ||
$self->respond_to(json => {json => {version => $user->last_login_version, seen => $user->feature_informed}},); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the duplicated json? Returning a hash is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check route implies you want to query the server for every page load. This sounds problematic. You better check while rendering the page - if you need the feature feature at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coolo thanks, will rework all these parts :)
my $user = $self->current_user; | ||
my $version = $self->param('version'); | ||
$user->update({last_login_version => $version}); | ||
my $seen = $self->param('seen'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what else would seen be? You want to the user to confirm he didn't see it? :)
IMO these 2 fields are duplicating each other. Assume I visited openqa last on version 3 and now it's 5. So I need 2 features to be presented. Once these are presented, they are gone - and should never appear again. So the last_login_version is enough to know the features to be presented.
lib/OpenQA/Schema/Result/Users.pm
Outdated
@@ -51,6 +51,14 @@ __PACKAGE__->add_columns( | |||
false_id => ['0', '-1'], | |||
default_value => '0', | |||
}, | |||
last_login_version => { | |||
data_type => 'text', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't make this a text version - make a list of features to be presented and store the number seen. And then have the user either ignore them all or go through them.
Codecov Report
@@ Coverage Diff @@
## master #1388 +/- ##
==========================================
+ Coverage 87.7% 87.73% +0.02%
==========================================
Files 104 105 +1
Lines 7580 7588 +8
==========================================
+ Hits 6648 6657 +9
+ Misses 932 931 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working on it, its is not ready yet.
templates/layouts/bootstrap.html.ep
Outdated
@@ -30,6 +30,7 @@ | |||
$(function() { | |||
setupForAll(); | |||
%= content_for 'ready_function' | |||
newFeature(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the appropriate way to accomplish this, but i have to somehow load the bootstrap-tour instructions when traversing through multiple pages. Example will follow soon.
335cf7d
to
1c5cf85
Compare
The dynamic notifications about new features are working now. There are still a few points which i have to improve, like the "I'm not interested" checkbox, which is doing nothing so far and making the feature javascript a bit more comfortable to maintain. I was thinking about getting those things done and the tests green. After that i would like to add a "real" tour. Feedback is welcomed :) |
I'm very much looking forward to the tour part for newcomers, and also it will for sure help us with poo#16374 at least on the webUI part. |
awesome video. Very nice. |
This part fails when i start the tests on my local machine with perl-5-24.1 and tests gets aborted with an error: make: *** [Makefile:75: checkstyle] Error 2 Looks it appears since 2ad797f lib/OpenQA/WebAPI/Plugin/Helpers.pm: Hash key with quotes at line 331, column 37. Avoid useless quotes. (Severity: 5) |
So fix these first, your room colleagues should be able to help you with that :-) |
a707035
to
b121fb5
Compare
@@ -51,6 +51,10 @@ __PACKAGE__->add_columns( | |||
false_id => ['0', '-1'], | |||
default_value => '0', | |||
}, | |||
feature_version => { | |||
data_type => 'integer', | |||
default_value => 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disabled the feature tour temporarly, since i dont have a real tour yet. Will do a real tour in the next step, but i thought it would be a good idea to make a merge of the branch possible.
lib/OpenQA/Schema.pm
Outdated
@@ -30,7 +30,7 @@ use OpenQA::Utils (); | |||
|
|||
# after bumping the version please look at the instructions in the docs/Contributing.asciidoc file | |||
# on what scripts should be run and how | |||
our $VERSION = 56; | |||
our $VERSION = 57; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reduced database number, since i pushed it more then once in the process, so currently it would be 57
t/fixtures/03-users.pl
Outdated
}, | ||
Users => { | ||
username => 'nobody', | ||
feature_version => 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I enabled the tour for the user nobody for the feature-tour ui test
874e88b
to
e7d4638
Compare
e7d4638
to
1de38eb
Compare
1de38eb
to
9ce22a3
Compare
So I'm ready with the base of this feature. Feature notifications are disabled, since i havent prepared proper ones yet. This will be the next step. |
initTour(featureVersion); | ||
|
||
//Parse html code as string to change the default layout of bootstrap tour | ||
function changeTemplate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you better put that HTML in a hidden block with proper indenting.
But I'll merge (and squash) in this stage.
commit 4c72a17 Author: Lukas Krause <lkrause@suse.de> AuthorDate: Sun Jul 30 18:35:50 2017 +0200 Commit: Stephan Kulow <coolo@kde.org> CommitDate: Sun Jul 30 18:35:50 2017 +0200 Dynamic feature infos for users (#1388) * Add bootstrap-tour to assetpack * Add javascript and controller files for features * Create API routes for feature notifications * Add subroutines to feature module * Change feature api route from public to user * Change json parsing in feature subroutine * Add prototyp of the feature jquery code * Add feature function to bootstrap template for multipage traversal * Improve feature javascript and add example tour * Add query for is user logged in * Add checkbox to suppress new feature notifications * Add a few comments * Fix avoid useless quotes test error in helpers module * Fix ui tests and add test for feature tour * Move a few things in feature javascript * Disable feature tour by seeting database entry to zero * Enable tour for user nobody in feature test * Get rid of unnecessary ajax call in feature javascript * Remove redundant api route * Adjust database and increase version number to 58
commit 4c72a17 Author: Lukas Krause <lkrause@suse.de> AuthorDate: Sun Jul 30 18:35:50 2017 +0200 Commit: Stephan Kulow <coolo@kde.org> CommitDate: Sun Jul 30 18:35:50 2017 +0200 Dynamic feature infos for users (#1388) * Add bootstrap-tour to assetpack * Add javascript and controller files for features * Create API routes for feature notifications * Add subroutines to feature module * Change feature api route from public to user * Change json parsing in feature subroutine * Add prototyp of the feature jquery code * Add feature function to bootstrap template for multipage traversal * Improve feature javascript and add example tour * Add query for is user logged in * Add checkbox to suppress new feature notifications * Add a few comments * Fix avoid useless quotes test error in helpers module * Fix ui tests and add test for feature tour * Move a few things in feature javascript * Disable feature tour by seeting database entry to zero * Enable tour for user nobody in feature test * Get rid of unnecessary ajax call in feature javascript * Remove redundant api route * Adjust database and increase version number to 58
Related to:
https://progress.opensuse.org/issues/16376