-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[full-ci] Add account creation time #40588
Conversation
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.
indent looks off in some places
3c7f24c
to
ae2f786
Compare
💥 Acceptance tests pipeline webUIFfSmoke-3-1-firefox-mariadb10.2-php7.4 failed. The build has been cancelled. |
On the UI, for existing users, the UI shows Creation Time "never". Maybe it would be better to have a special-case text in that case (when Creation Time" is Unix timestamp 0), and say "unknown". |
|
Locally, I had to make sure that the migration ran, so that the accounts table change happened. I pushed a commit to do the smallest version bump in this PR. I also put Locally the reported failing scenario runs for me - I will dig more after CI finishes. |
Should be addressed now. |
works for me. |
https://drone.owncloud.com/owncloud/core/37752/158/17
|
https://drone.owncloud.com/owncloud/core/37752/119/17 In tests/acceptance/run.sh there is special code that tries to set the browser window big enough:
The problem is that the UI testing framework (Behat+Mink+selenium) does not successfully auto-scroll elements into view during test execution. When a UI is wide, it is easy to have elements that are off the right-hand side of the displayed area, and so are not clickable. The User Management page is wide, (lots of columns) and gets wider with this change. The failing scenario trys to click the "Resend invitation email" button, and that is way off on the right-hand edge. I am running Firefox, and I can do it manually, but I have to scroll the window all the way to the right. IMO it is not worth trying to run this scenario on Firefox and not worth trying to adjust the screen size etc. I will skip it on Firefox. |
I added a couple of acceptance tests that check that creation time is returned in the provisioning API, as is visible on the webUI. They pass locally for me. I expect that CI will pass. |
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.
LGTM, works locally.
Needs a developer review. The code looks OK to me, but it would be good to have another review.
@@ -102,6 +102,14 @@ public function updateLastLoginTimestamp() { | |||
return true; | |||
} | |||
|
|||
/** | |||
* Reset user creation time (return 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.
Indentation is wrong here
lib/private/User/User.php
Outdated
* | ||
* @return void | ||
*/ | ||
public function setCreationTime() { |
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.
Should this method be implemented? I mean, I could get an user instance and update the creation time, but I don't think we should allow that.
lib/public/IUser.php
Outdated
* @return void | ||
* @since 10.12.0 | ||
*/ | ||
public function setCreationTime(); |
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.
As said, I don't think this method should be public.
The creation time is handled by the account class, and the user has a dependency on the account in order to get the creation time, but it doesn't imply the user should be allowed to set the creation time.
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.
Mechanisms like the user sync command needs this?
But I guess you are right - this field should be set internally on creation of an account.
I'll take care.
THX @jvillafanez
cc48aa1
to
7be755d
Compare
SonarCloud Quality Gate failed. |
Note: unit tests for issue owncloud/configreport#184 PR owncloud/configreport#185 had to be adjusted to include mention of this new account data item. |
Confirmed in 10.12.0-rc.2 |
Description
Add account creation time
Related Issue
Motivation and Context
Add account creation time in the
oc_accounts
table so that this info can be displayed over the Users page. Also extended the users provisioning API and theuser:list
occ command to display this info.How Has This Been Tested?
Manually:
Test 1 :
Creation Time
column correctly displays this infoTest 2 :
user:sync
command and check that theCreation Time
column correctly displays this infoTest 3 :
-Share with a guest user (create the user) and check that the
Creation Time
column correctly displays this infoTest 4 :
user:add
occ command to create a new user and check that theCreation Time
column correctly displays this infoTest 5 :
Test 6 :
user:list
occ command to show user's attributes and check that this info is correctly displayed:Types of changes
Checklist: