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

Status Feature #21186

Merged
merged 1 commit into from
Jul 31, 2020
Merged

Status Feature #21186

merged 1 commit into from
Jul 31, 2020

Conversation

georgehrke
Copy link
Member

@georgehrke georgehrke commented Jun 2, 2020

fixes #20931

Known issues:

  • Current check for emoji uses mb_strlen and hence does not support Emoji Modifier Sequences nor Zero Width Joiner. Need to dig into the standard to find a better way to check if it's exactly one emoji.
    Fixed in 777f9f6
  • add check if DB supports 4byte

Blocked by:

@georgehrke georgehrke added the 2. developing Work in progress label Jun 2, 2020
@georgehrke georgehrke self-assigned this Jun 2, 2020
@georgehrke georgehrke force-pushed the feature/20931/user_status branch 4 times, most recently from 5d9711b to a6b64d7 Compare June 3, 2020 15:57
@georgehrke georgehrke force-pushed the feature/20931/user_status branch 2 times, most recently from fe1abbd to 777f9f6 Compare June 3, 2020 16:11
@georgehrke georgehrke force-pushed the feature/20931/user_status branch 4 times, most recently from 7df82f8 to e6d8b70 Compare June 8, 2020 08:43
@georgehrke georgehrke force-pushed the feature/20931/user_status branch 2 times, most recently from 84a8b47 to ab8cdbc Compare June 23, 2020 14:33
apps/user_status/appinfo/app.php Outdated Show resolved Hide resolved
apps/user_status/appinfo/info.xml Outdated Show resolved Hide resolved
*/

return [
'ocs' => [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎

apps/user_status/img/app.svg Outdated Show resolved Hide resolved
apps/user_status/lib/Controller/AStatusController.php Outdated Show resolved Hide resolved
apps/user_status/tests/Integration/Application/AppTest.php Outdated Show resolved Hide resolved
@georgehrke georgehrke force-pushed the feature/20931/user_status branch 2 times, most recently from eae589a to ae2b0c9 Compare July 7, 2020 14:37
@jancborchardt
Copy link
Member

jancborchardt commented Jul 7, 2020

Some first quick design review @georgehrke just pasting here since I’m about to leave the office (and internet connection with that ;)

  • App icon: Just use away icon?
  • Default status is "Invisible"? Bit unclear
  • Wording: "Set a status" → "Set status"
  • After state is set for the first time, display the value like "Online", not still "Set status"
  • Status title line is a bit off to the right
  • "Clear status after" instead of "Clear status at" ("after today", "after 30 minutes", "after this week" etc)
  • "Don’t clear" vs "Never", should be the same wording
  • Manually setting the expiration results in "Select option" to be displayed in the select
  • Emoji not reflected in the user menu, should be before the text?

@georgehrke

This comment has been minimized.

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some quick tests and the code looks sane 👍

@georgehrke georgehrke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 31, 2020
@georgehrke
Copy link
Member Author

Rebased to fix merge conflicts.
Will merge once CI is done.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autoloader check and commit missing for the app

icon="icon-rename"
:close-after-click="true"
@click.prevent.stop="openModal">
{{ $t('user_status', 'Set custom status') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use:

Suggested change
{{ $t('user_status', 'Set custom status') }}
{{ t('user_status', 'Set custom status') }}

everywhere. why is this different in this app?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we are doing it wrong everywhere.

https://vuejs.org/v2/cookbook/adding-instance-properties.html#The-Importance-of-Scoping-Instance-Properties

If you add properties to the prototype, you are supposed to prefix them with $ to avoid conflicts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also an issue about it here: nextcloud-libraries/nextcloud-vue#586

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bothers me to have it "fixed" in one app. For the sake of maintainability I would prefer to have t for now until we update all usages at once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plenty of apps are doing it correctly already. Calendar, Tasks, Privacy, ...

@nickvergessen
Copy link
Member

The invisible icon is black on dark background when dark theme is used.

@georgehrke
Copy link
Member Author

The invisible icon is black on dark background when dark theme is used.

Added to follow up list in PR description.

@nickvergessen

This comment has been minimized.

@nickvergessen

This comment has been minimized.

@georgehrke
Copy link
Member Author

/compile amend /

@faily-bot
Copy link

faily-bot bot commented Jul 31, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 31276: failure

mariadb10.4-php7.3

Show full log
There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

--

There was 1 failure:

1) Test\Files\Cache\CacheTest::testGetById
null does not match expected type "array".

/drone/src/tests/lib/Files/Cache/CacheTest.php:495

integration-provisioning-v1

  • build/integration/features/provisioning-v1.feature:322
Show full log
  Scenario: get enabled apps                           # /drone/src/build/integration/features/provisioning-v1.feature:322
    Given As an "admin"                                # FeatureContext::asAn()
[Fri Jul 31 13:53:39 2020] 127.0.0.1:51526 [200]: /ocs/v1.php/cloud/apps?filter=enabled
    When sending "GET" to "/cloud/apps?filter=enabled" # FeatureContext::sendingTo()
    Then the OCS status code should be "100"           # FeatureContext::theOCSStatusCodeShouldBe()
    And the HTTP status code should be "200"           # FeatureContext::theHTTPStatusCodeShouldBe()
    And apps returned are                              # FeatureContext::theAppsShouldBe()
      | accessibility           |
      | cloud_federation_api    |
      | comments                |
      | contactsinteraction     |
      | dashboard               |
      | dav                     |
      | federatedfilesharing    |
      | federation              |
      | files                   |
      | files_sharing           |
      | files_trashbin          |
      | files_versions          |
      | lookup_server_connector |
      | provisioning_api        |
      | settings                |
      | sharebymail             |
      | systemtags              |
      | theming                 |
      | twofactor_backupcodes   |
      | updatenotification      |
      | user_ldap               |
      | viewer                  |
      | workflowengine          |
      | files_external          |
      | oauth2                  |
      Failed asserting that two arrays are equal.
      --- Expected
      +++ Actual
      @@ @@
      -    23 => 'viewer'
      -    24 => 'workflowengine'
      +    23 => 'user_status'
      +    24 => 'viewer'
      +    25 => 'workflowengine'
[Fri Jul 31 13:53:39 2020] Login failed: 'user0' (Remote IP: '127.0.0.1')
[Fri Jul 31 13:53:39 2020] 127.0.0.1:51562 [401]: /remote.php/webdav/myFileToComment.txt
[Fri Jul 31 13:53:39 2020] 127.0.0.1:51564 [207]: /remote.php/dav/systemtags/
[Fri Jul 31 13:53:39 2020] Login failed: 'user0' (Remote IP: '127.0.0.1')
[Fri Jul 31 13:53:39 2020] 127.0.0.1:51594 [401]: /remote.php/webdav/myFileToTag.txt
[Fri Jul 31 13:53:39 2020] 127.0.0.1:51596 [404]: /remote.php/dav/addressbooks/users/admin/MyAddressbook
[Fri Jul 31 13:53:40 2020] 127.0.0.1:51616 [404]: /remote.php/dav/calendars/admin/MyCalendar

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦈 let's go

Signed-off-by: Georg Ehrke <[email protected]>
@georgehrke georgehrke merged commit 8e01ff1 into master Jul 31, 2020
@georgehrke georgehrke deleted the feature/20931/user_status branch July 31, 2020 15:25
@jancborchardt
Copy link
Member

Wooohooo congrats @georgehrke! :)))

@juliushaertl we can continue with the Status integration in Dashboard. ;)

@georgehrke georgehrke mentioned this pull request Aug 5, 2020
MorrisJobke added a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <[email protected]>
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <[email protected]>
MorrisJobke added a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <[email protected]>
MorrisJobke added a commit to nextcloud/documentation that referenced this pull request Aug 10, 2020
* LoadAdditionalScripts (@rullzer) - nextcloud/server#16641
* LoadViewerEvent (@skjnldsv) - nextcloud/viewer#271
* RegisterDirectEditorEvent (@juliushaertl) - nextcloud/server#17625
* typed events for files scanner (@ChristophWurst) - nextcloud/server#18351
* typed events for group mangement (@ChristophWurst) - nextcloud/server#18350
* AddContentSecurityPolicyEvent (@rullzer) - nextcloud/server#15730
* UserLiveStatusEvent (@georgehrke) - nextcloud/server#21186
* password_policy events (@ChristophWurst) - nextcloud/server#18019
* AddFeaturePolicyEvent (@rullzer) - nextcloud/server#16613
* ShareCreatedEvent (@rullzer) - nextcloud/server#18384
* LoadSettingsScriptsEvent (@blizzz) - nextcloud/server#21475
* flow events (@rullzer) - nextcloud/server#18535

Signed-off-by: Morris Jobke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: status
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status overview
9 participants