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

Add Warning Dialog when HumHub doesn't support push #188

Closed
luke- opened this issue Jun 4, 2024 · 23 comments
Closed

Add Warning Dialog when HumHub doesn't support push #188

luke- opened this issue Jun 4, 2024 · 23 comments
Assignees

Comments

@luke-
Copy link
Collaborator

luke- commented Jun 4, 2024

The app should display a warning dialogue if the selected HumHub installation does not have a configured push module installed.

The status can be queried via: humhub/fcm-push#37

Warn text could be, for example:

‘No push notifications are available for the selected HumHub installation. Contact the administrator for more information.’

@Semir1212 can you check the text please?

@Semir1212
Copy link

@luke-

"Push notifications are not supported by this installation."

@luke-
Copy link
Collaborator Author

luke- commented Jun 7, 2024

Route is /fcm-push/status

@PrimozRatej PrimozRatej self-assigned this Jul 22, 2024
@PrimozRatej
Copy link
Collaborator

Hey @luke-,

There is a minor issue here. The community page exposes ${manifest.baseUrl}/fcm-push/status to the public, allowing data access without authentication. For private instances like sometestproject12345 and Omrade, which require authentication, the status is inaccessible without logging in.

We use the dio package for communication with the REST API, which is not authenticated because authentication occurs inside the webview, where the user state is managed. The webview does not support advanced web client functionalities. It is possible to parse the returned HTML and extract JSON, but I would like to avoid that.

I would use JS message method, similar to how we did for registering the FCM token.

@marc-farre
Copy link
Collaborator

@luke- @PrimozRatej Why not displaying this warning to logged admins only?
I think the most important is to inform them their HumHub instance is not well configured for the mobile app and how to do it.

It might even be better not displaying the warning to non-admins, because maybe some communities will want to use the app without push feature.

What do you think?

@ArchBlood
Copy link

@luke- @PrimozRatej Why not displaying this warning to logged admins only? I think the most important is to inform them their HumHub instance is not well configured for the mobile app and how to do it.

It might even be better not displaying the warning to non-admins, because maybe some communities will want to use the app without push feature.

What do you think?

I believe this would also work better when humhub/fcm-push#24 is implemented, I've also contributed a modified version of the P/R within this issue to implement the grant statuses with @luke-'s points in mind and can say overall it works rather well, but it could be updated to also handle this issue.

@luke-
Copy link
Collaborator Author

luke- commented Jul 30, 2024

@luke- @PrimozRatej Why not displaying this warning to logged admins only? I think the most important is to inform them their HumHub instance is not well configured for the mobile app and how to do it.

It might even be better not displaying the warning to non-admins, because maybe some communities will want to use the app without push feature.

What do you think?

We can also do it that way.

But if we only want to implement a warning for admins, we should implement this on the HumHub module side in the fcm-push module.

@marc-farre
Copy link
Collaborator

marc-farre commented Jul 30, 2024

on the HumHub module side

Maybe we could use the HumHub-Mobile user agent marker to detect when we're viewing HumHub from the mobile app: #182 (comment)

We could take this opportunity to create:

  • A new helper in humhub\modules\ui\helpers, e.g. UserAgentHelper, with static methods such as:
  • A new JS module, e.g. ui.userAgent (even if we don't need it for the moment, but could be useful for some modules), with methods such as:
    • isHumHubMobileApp which will return /HumHub-Mobile/i.test(navigator.userAgent)
    • isMobileBrowser which will return /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent)
    • maybe other similar methods
  • A new class or data attribute on the html or body HTML tag, e.g. is-humhub-mobile-app?

Then, we could add the warning to the IncompleteSetupWarning widget?

@luke-
Copy link
Collaborator Author

luke- commented Jul 30, 2024

@marc-farre We already have some helper here:
https://github.com/humhub/fcm-push/blob/master/helpers/MobileAppHelper.php#L58

Maybe we could copy that, for now somewhere into the core.

At some point we should think about integrating at least some app parts of the fcm-push module directly into the core.

@marc-farre
Copy link
Collaborator

@luke- done:

@marc-farre
Copy link
Collaborator

@luke- Do you want me to add the warning to the IncompleteSetupWarning widget using the new DeviceDetectorHelper?

@luke-
Copy link
Collaborator Author

luke- commented Aug 2, 2024

@marc-farre Yes, that would be fine! Thanks!

@marc-farre
Copy link
Collaborator

@luke- PR humhub/humhub#7166

What do you think about this view?
image

I had to rework the "Open documentation" button, because it's not the same link for the push notification configuration.
I didn't find a better solution than display twice the same button for the 2 cron job warning.

@luke-
Copy link
Collaborator Author

luke- commented Aug 12, 2024

@marc-farre Thanks, the PR looks good!

@Eladnarlea Can you please check the buttons?

@Eladnarlea
Copy link

Eladnarlea commented Aug 13, 2024

@luke- I would suggest to do it in the style of the alert/prerequisites design.

In case you do not wish to implement it yet, we could only implement the icon-button help-link first, but also add one icon-button view documentation to the bottom of the window

Screenshot 2024-08-13 at 1 22 40 PM

EDIT: in terms of layout and sizes I am unsure how your suggestion would look like, implemented, @marc-farre. I find it difficult to evaluate your design & button positions for this reason.
Could you please provide the whole screen for mobile and desktop, @marc-farre ? :)

@luke-
Copy link
Collaborator Author

luke- commented Aug 13, 2024

@Eladnarlea Thanks, unfortunately I don't have time for the Requirement Redesign topic at the moment. But I would like to take a closer look at the design here before implement.

So I would suggest that we first add buttons (or a simple link) and not change the design at all. Later in the requirement redesign we can also consider this sidebar element.

@marc-farre Can you please add a simple plain link regarding the app for now. We'll hopefully redesign this soon.

@marc-farre
Copy link
Collaborator

marc-farre commented Aug 15, 2024

@luke- @Eladnarlea commit humhub/humhub@b452871

image

Or did you want to keep the existent "Documentation" button for cron jobs and add a warning about the mobile app bellow?

@luke-
Copy link
Collaborator Author

luke- commented Aug 15, 2024

The help buttons looks a bit strange vor me. Maybe just?
image

@marc-farre
Copy link
Collaborator

@luke- Commit humhub/humhub@77aedf1

image

@Eladnarlea
Copy link

Eladnarlea commented Aug 16, 2024

@luke- in order to unify the software, could we please try to use the unified version like in the prerequisites used, already? Meaning using the [icon] Help "ghost button?

I think, what you didn't like about the help button was not the button itself, rather the styling of the link-icon- If @marc-farre could just make it the same font-size as the help font size itself, as well as using capitalisation "H"elp AND for now maybe using fnot-weight: 500 for Help?
I think that would do the job. @luke- what do you reckon?

@luke-
Copy link
Collaborator Author

luke- commented Aug 16, 2024

@Eladnarlea I haven't had a closer look at the new Prereq design yet... so I don't want to already start implementing it in this unrelated task.

@marc-farre
Copy link
Collaborator

@luke- Is my PR fine, or you want changes on it?

@luke-
Copy link
Collaborator Author

luke- commented Aug 16, 2024

@marc-farre Thanks, it's fine. But there will be UI changes as soon we refactoring the PreReq page :-)

@marc-farre
Copy link
Collaborator

@luke- I close this issue. We can open a new one about UI refactoring.

github-merge-queue bot pushed a commit to humhub/humhub that referenced this issue Oct 28, 2024
#7166)

* When browsing HumHub from the mobile app, add a warning to the dashboard (IncompleteSetupWarning widget) if HumHub doesn't support push notifications

* Add PR ID to changelog

* Fix: remove test condition

* humhub/app#188 (comment)

* humhub/app#188 (comment)

---------

Co-authored-by: Lucas Bartholemy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

6 participants