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

weather status used in dashboard #22124

Merged
merged 3 commits into from
Aug 20, 2020
Merged

weather status used in dashboard #22124

merged 3 commits into from
Aug 20, 2020

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Aug 6, 2020

I did more or less the same as the user status. It's inserted in dashboard statuses.

There's a new weather_status module that needs to be built.

It's an independent app in case we want to use it in Calendar for example.
It uses api.met.no to get forecasts and nominatim.openstreetmap.org to search and resolve locations.

One thing I'm not sure about: How to make sure the weather status script is loaded when browsing dashboard page. I did it in the dashboard template...

Todo:

  • let the user choose to use the address set in personal settings to determine location and use it for weather
  • remove debug logs
  • improve backend error management

weather1

weather2

@jancborchardt
Copy link
Member

cc @berdosi of the Weather app, maybe you are interested to take a look at this? :) This is explicitly direct integration only (in Dashboard and possibly in Calendar), and it would still make sense to have a bespoke Weather app.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Aaaaawesome! :)

Copy link
Contributor

@splitt3r splitt3r left a comment

Choose a reason for hiding this comment

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

Looks great 😍 small nitpicks

@splitt3r
Copy link
Contributor

splitt3r commented Aug 9, 2020

The GitHub Actions failures look related but easy to fix 😁

@julien-nc julien-nc force-pushed the enh/weather-status branch from caf4c24 to 44a7e8e Compare August 9, 2020 08:42
@splitt3r
Copy link
Contributor

splitt3r commented Aug 9, 2020

I think you need to add your new app to

. To make integration-provisioning-v1 pass. And maybe also https://github.com/nextcloud/server/blob/7b91cb502907f5ebc3323567422dbbe1c9537d62/core/shipped.json to enable it by default?

@julien-nc
Copy link
Member Author

@splitt3r Thanks for the hints. I can't figure out how to get rid of this last CI-drone failure... Any idea?

@juliusknorr juliusknorr requested a review from rullzer August 10, 2020 12:54
@splitt3r
Copy link
Contributor

@splitt3r Thanks for the hints. I can't figure out how to get rid of this last CI-drone failure... Any idea?

I think that one is unrelated. Looks like the test fails in all recent MRs e.g. #22151 (comment).

@julien-nc
Copy link
Member Author

@juliushaertl History and code cleanup done, rebased on master, ready to merge 🎉.

@go2sh
Copy link
Contributor

go2sh commented Aug 10, 2020

You should Think about caching or do the weather requests on the client. Otherwise you might violate the tos of the api, especially on big instances. And thus might get blocked etc.

@julien-nc
Copy link
Member Author

julien-nc commented Aug 11, 2020

You should Think about caching or do the weather requests on the client. Otherwise you might violate the tos of the api, especially on big instances. And thus might get blocked etc.

Thanks for the feedback.
I'd like to avoid letting the client do the requests to avoid changing the CSP.

With one request per hour, even if the request is cached on the client, it would be useless as we want a forecast update every hour.

I figured one request per hour per user was not so much, even with a large number of users.

If caching was done on server side, it would factorize all requests for the same location. I'll probably do it but it's not trivial, it requires to store results, clean cache at some point...

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
@go2sh
Copy link
Contributor

go2sh commented Aug 11, 2020

How do you limit to 1 request per hour on the server. Through client caching?

@nickvergessen
Copy link
Member

Looks weird when they touch, I think:

Bildschirmfoto von 2020-08-19 16-08-13

@julien-nc
Copy link
Member Author

Looks weird when they touch, I think:

Spacing between statuses will be fixed in dashboard PR as mentioned earlier (#22124 (comment))

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.

Some PHP coding style comments.

Also I noticed: my location is New York, my language en US and my locale en US. I should see the temperature in °F ? but it just says 23° so I grab a jacket and go fetch my skiers? At least 23° C needs to be shown, but if possible °F

apps/weather_status/appinfo/info.xml Outdated Show resolved Hide resolved
apps/weather_status/appinfo/info.xml Outdated Show resolved Hide resolved
apps/weather_status/lib/Service/WeatherStatusService.php Outdated Show resolved Hide resolved
apps/weather_status/lib/Service/WeatherStatusService.php Outdated Show resolved Hide resolved
apps/weather_status/lib/Service/WeatherStatusService.php Outdated Show resolved Hide resolved
apps/weather_status/lib/Service/WeatherStatusService.php Outdated Show resolved Hide resolved
apps/weather_status/src/App.vue Outdated Show resolved Hide resolved
@julien-nc
Copy link
Member Author

Recent changes:

  • Elevation is now more precise (to get more precise temperature)
  • Temperature is now displayed in Fahrenheit for those locales : 'en_US', 'en_MH', 'en_FM', 'en_PW', 'en_KY', 'en_LR'
  • Bump to v1.0.0
  • More details in info.xml summary
  • Code style fixed

Rebased on master and commits squashed.

@MorrisJobke
Copy link
Member

@eneiluj Once rebased on current master Psalm will also be more happy because #22304 was merged.

@MorrisJobke
Copy link
Member

The remaining complain is just about more fixed stuff, just ignore for now. 🚀 good go

@julien-nc
Copy link
Member Author

@MorrisJobke There should be a CI job checking the validity of the CI jobs check code 😉 Thanks for the hint.

@MorrisJobke
Copy link
Member

@MorrisJobke There should be a CI job checking the validity of the CI jobs check code 😉 Thanks for the hint.

I have already something in mind to avoid this mess, but one step after the other :)

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.

🐘

apps/weather_status/src/App.vue Outdated Show resolved Hide resolved
Julien Veyssier and others added 2 commits August 20, 2020 00:21
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member

Rebased now that the Dashboard background is merged, and fixed the small design issues (statuses touching, font size being too small, them not wrapping on mobile, background of weather status button looking different).

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Good from my side! :)

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@faily-bot
Copy link

faily-bot bot commented Aug 20, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32029: failure

acceptance-app-files

  • tests/acceptance/features/app-files.feature:262
Show full log
  Scenario: unmarking a file as favorite causes the file list to be sorted again                          # /drone/src/tests/acceptance/features/app-files.feature:262
    Given I am logged in                                                                                  # LoginPageContext::iAmLoggedIn()
    And I create a new folder named "A name alphabetically lower than welcome.txt"                        # FileListContext::iCreateANewFolderNamed()
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    And I close the details view                                                                          # FilesAppContext::iCloseTheDetailsView()
    And I see that the details view is closed                                                             # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
    And I mark "welcome.txt" as favorite                                                                  # FileListContext::iMarkAsFavorite()
    And I see that "welcome.txt" is marked as favorite                                                    # FileListContext::iSeeThatIsMarkedAsFavorite()
    And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    When I unmark "welcome.txt" as favorite                                                               # FileListContext::iUnmarkAsFavorite()
    Then I see that "welcome.txt" is not marked as favorite                                               # FileListContext::iSeeThatIsNotMarkedAsFavorite()
      Not favorited state icon for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()

@MorrisJobke MorrisJobke merged commit 1f4c029 into master Aug 20, 2020
@MorrisJobke MorrisJobke deleted the enh/weather-status branch August 20, 2020 06:05
@skjnldsv
Copy link
Member

@eneiluj
Geolocation doesn't work for me
I have the same as nextcloud/maps#229

Geolocation has been disabled in this document by Feature Policy

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Aug 20, 2020
@rullzer
Copy link
Member

rullzer commented Aug 20, 2020

@eneiluj
Geolocation doesn't work for me
I have the same as nextcloud/maps#229

Geolocation has been disabled in this document by Feature Policy

AH yes you need to listen to the event when you show this and enable it. Let me see if I can do that.

@jancborchardt
Copy link
Member

Congrats on the merge! :) Finally it all works in concert together!

@julien-nc
Copy link
Member Author

@jancborchardt thanks! so nice to use everything without rebuilding 😁. the design is simply awesome. thanks a lot!

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: dashboard integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants