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

OSM Teams integration #5575

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

willemarcel
Copy link
Contributor

This PR allows users to get members and managers from an OSM Teams team.

How to test:

  • Create a new OAuth2 app on https://auth.mapping.team and copy the CLIENT_ID and CLIENT_SECRET. Use it to configure your TM instance, setting the following environment variables:
OSM_TEAMS_API_URL='https://dev.mapping.team' # it should use the dev instance now
OSM_TEAMS_CLIENT_ID=foo
OSM_TEAMS_CLIENT_SECRET=foo
  • Go to the team management page, login to OSM Teams, choose an OSM Teams team to sync from and create a new team.

Some screenshots

Team page

image

Team selection modal

image

Selected team details

image

Managers and members imported

image

Resync action button

image

Copy link
Contributor

@HelNershingThapa HelNershingThapa left a comment

Choose a reason for hiding this comment

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

A text under Sync with OSM Teams reads, “Authorize Tasking Manager to…”. The button text reads "Authenticate OSM Teams," and the popup displays "OSM Teams authentication successful" on success. To maintain consistency, could we update the button text to match the message displayed on success or vice versa?

We noticed that adding a member to the team was possible via a join request. To avoid any unexpected issues, I think we should consider disabling all feature implementations related to modifying the members/managers list of a team that has been synced with OSM Teams. Since the sync is one-way, we’d want to ensure that any modifications to the members list for a team synced with OSM Teams are only done through the OSM Teams portal. Might as well create another team type for OSM Teams team on the backend.

Should we also update the user interface to add a visual clue for team managers indicating that modifying the members is only available through the OSM Teams portal? Although there is already a text "Synced with OSM Teams" under members, clarifying that modifying the members is only available through the OSM Teams portal would be helpful. This would help prevent any confusion and ensure that team managers know the restrictions.

Users who could not be synced with the team due to their account being unavailable on the Tasking Manager are not displayed. Could you add this information to the user interface? Otherwise, it will be hard for managers to know who are the ones that weren’t imported.

Could you also add some info/docs on what the callback URL should be while creating an app on OSM Teams?

frontend/src/components/teamsAndOrgs/teamSync.js Outdated Show resolved Hide resolved
frontend/src/views/teams.js Outdated Show resolved Hide resolved
frontend/src/views/teams.js Outdated Show resolved Hide resolved
@willemarcel
Copy link
Contributor Author

willemarcel commented Mar 31, 2023

We noticed that adding a member to the team was possible via a join request. To avoid any unexpected issues, I think we should consider disabling all feature implementations related to modifying the members/managers list of a team that has been synced with OSM Teams. Since the sync is one-way, we’d want to ensure that any modifications to the members list for a team synced with OSM Teams are only done through the OSM Teams portal. Might as well create another team type for OSM Teams team on the backend.

I think it would be enough to switch the team to be "invite only" after the first osm teams sync, and disable the change later. What do you think?

Should we also update the user interface to add a visual clue for team managers indicating that modifying the members is only available through the OSM Teams portal?

I improved the text. Let me know if it is good that way.

Could you add this information to the user interface? Otherwise, it will be hard for managers to know who are the ones that weren’t imported.

I've added it too

Could you also add some info/docs on what the callback URL should be while creating an app on OSM Teams?

The callback url should be FRONTEND_URL + "/osmteams-authorized"

@willemarcel
Copy link
Contributor Author

I think it would be enough to switch the team to be "invite only" after the first osm teams sync, and disable the change later.

@HelNershingThapa this is done now.

@sonarcloud
Copy link

sonarcloud bot commented Apr 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ramyaragupathy ramyaragupathy self-assigned this Apr 19, 2023
@ramyaragupathy
Copy link
Member

@willemarcel - could you also add a field type in DB to track OSM teams? May be include one more option in join type. Since there is no way to send an invite via TM and the team management will entirely happen on OSM Teams platform, it is best we capture it here and accordingly control the options for the user. It is also easier for us to track the number of teams that are managed externally.

cc @HelNershingThapa @Aadesh-Baral

@HelNershingThapa
Copy link
Contributor

@willemarcel, could you also add test cases for the changes made? We've been focusing on test coverage lately to ensure the code is as reliable and bug-free as possible. We'd love it if you added test cases for your PR to help us achieve our goal.

@willemarcel willemarcel force-pushed the feature/osm-teams-integration branch 2 times, most recently from acfdf31 to af8dbcb Compare July 6, 2023 22:12
@willemarcel willemarcel force-pushed the feature/osm-teams-integration branch from af8dbcb to cb57994 Compare July 7, 2023 18:13
@willemarcel
Copy link
Contributor Author

@ramyaragupathy @HelNershingThapa I've added a new team join method named OSM_TEAMS. I've updated the UI of the team membership page with that new condition too. I also added the tests and rebased it with develop branch.

The build is failing in the CI, but I don't know the reason...

@HelNershingThapa
Copy link
Contributor

The build is failing in the CI, but I don't know the reason...

It seems that the build failure might be connected to a potential conflict in the sequence of CSS bundle creation. I came across a relevant StackOverflow comment that discussed a similar problem. However, I currently don't have a specific solution or workaround to address the issue we're looking at.

@HelNershingThapa
Copy link
Contributor

hi @willemarcel. Just checking in, have you been able to debug the issue?

@willemarcel
Copy link
Contributor Author

@HelNershingThapa The build works on my machine... Could you check if you can build it too?

@HelNershingThapa
Copy link
Contributor

The build gets generated on my machine too; with the same warnings though.

@ramyaragupathy
Copy link
Member

@eternaltyro anything that we have to configure in our CI setup? cc @willemarcel @HelNershingThapa

@sonarcloud
Copy link

sonarcloud bot commented Jul 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@ramyaragupathy ramyaragupathy removed their assignment Jul 25, 2023
@HelNershingThapa
Copy link
Contributor

The build passed successfully upon removing the commit responsible for implementing lazy loading for routes in the try/5575-without-route-splitting branch. This has pointed toward the lazy loading implementation as a potential culprit behind the failure. We might need to investigate further into how the lazy loading implementation interacts with the build process, specifically the workaround commit 013dd36.

On another note, I attempted to increase the heap size incrementally from 2GB to 8GB using react-scripts --max_old_space_size=XXXX build, but unfortunately, this did not resolve the problem. The build continued to fail in the try/5575-osm-teams-circleci branch. So I think we can rule out the memory constraints as the root cause?

@willemarcel
Copy link
Contributor Author

The build passed successfully upon removing the commit responsible for implementing lazy loading for routes in the try/5575-without-route-splitting branch. This has pointed toward the lazy loading implementation as a potential culprit behind the failure. We might need to investigate further into how the lazy loading implementation interacts with the build process, specifically the workaround commit 013dd36.

In the past, I only used lazy loading in some components that had big size dependencies, like mapbox-gl and chartjs, some pages that were not used by most users, like the Management section, and some components used in many places, like the projects cards. Furthermore, as the application is cached by the service worker, the lazy loading is not helpful in most cases.

@HelNershingThapa
Copy link
Contributor

@tsmock, could you offer some assistance here? You might have some insight into addressing the CSS issue we're encountering.

@tsmock
Copy link
Contributor

tsmock commented Aug 23, 2023

Looking at https://app.circleci.com/pipelines/github/hotosm/tasking-manager?branch=try%2F5575-osm-teams-circleci (let me know if this is the wrong branch), it does look like an OOM issue in yarn build.

The "large" executors have 8g ram. I tested on a 4GB Raspberry Pi (nodejs 18, 8GB swapfile, node 18 with NODE_OPTIONS=--openssl-legacy-provider), and it succeeded. 4gb should be the default max_old_space_size value.

Using /usr/bin/time -f "%P %M" yarn build, the following data was obtained:

Type Time % CPU Memory (kb)
No polyfills, updated react-scripts dependencies 925s 142% 1938224kb
Polyfills, updated react-scripts dependencies 1339s 210% 1833852kb
base 1348s 198% 1807180kb
Reverted route splitting 543s 207% 1828924kb

Just to double-check the memory usage, I wrote a systemd unit for building the TM, as follows

# Put this in ${HOME}/.config/systemd/user/tasking-manager-build.service
[Unit]
Description=Build HOT TM with specific resource constraints

[Service]
Type=simple
# Point this at the frontend directory of the tm. %h is replaced by the user home directory.
WorkingDirectory=%h/workspace/tasking-manager/frontend

# Allows the build to work on Node 18, has to be commented out if using node 16
# Environment=NODE_OPTIONS=--openssl-legacy-provider
# Comment in/out depending upon whether or not you want to test with a specific node version
# Point it at the bin folder from a nodejs tarball
Environment=PATH=%h/node16/bin:/bin:/usr/bin
Environment=CI=true
Environment=GENERATE_SOURCEMAP=false

ExecStart=/usr/bin/yarnpkg build

MemoryAccounting=true
MemoryMax=2G
# Only needed to avoid having swap take care of higher usage than the max. This can be disabled if no swap is available.
MemorySwapMax=0


[Install]
WantedBy=default.target

Pre-revert of route splitting, the service got killed when it ran out of memory.
Post-revert of route splitting, the service still got killed when it ran out of memory.

Increasing the MemoryMax to 4G resolved that issue. I have no clue why it is running out of memory on the CI.

For the CSS issue referenced, I'm looking into it.

@tsmock
Copy link
Contributor

tsmock commented Aug 23, 2023

I've done a bit of investigating. Here are some possible solutions:

  1. Move all the import ".*.css"; into the index.html, index.js, or App.js
  2. Apply the following (note: I had reverted 013dd36, but I don't think that is necessary)
diff --git a/frontend/src/views/projectEdit.js b/frontend/src/views/projectEdit.js
index fb992c5ca..6ad3b9b27 100644
--- a/frontend/src/views/projectEdit.js
+++ b/frontend/src/views/projectEdit.js
@@ -6,10 +6,10 @@ import { FormattedMessage } from 'react-intl';
 
 import messages from './messages';
 import projectEditMessages from '../components/projectEdit/messages';
+import { PriorityAreasForm } from '../components/projectEdit/priorityAreasForm';
 import { DescriptionForm } from '../components/projectEdit/descriptionForm';
 import { InstructionsForm } from '../components/projectEdit/instructionsForm';
 import { MetadataForm } from '../components/projectEdit/metadataForm';
-import { PriorityAreasForm } from '../components/projectEdit/priorityAreasForm';
 import { ImageryForm } from '../components/projectEdit/imageryForm';
 import { PermissionsForm } from '../components/projectEdit/permissionsForm';
 import { SettingsForm } from '../components/projectEdit/settingsForm';

According to

@kaditya97
Copy link
Member

Having issue while trying to setup this PR locally

My local environment variables:
OSM_TEAMS_AUTH_DOMAIN='https://auth.mapping.team'
OSM_TEAMS_API_URL='https://mapping.team'
OSM_TEAMS_CLIENT_ID=--
OSM_TEAMS_CLIENT_SECRET=--
TM_OSM_TEAMS_REDIRECT_URI=http://127.0.0.1:3000/osmteams-authorized

The ui shows for sync with osm teams while creating new team
Screenshot 2024-03-28 101151

The authentication with auth.mapping.team works

Screenshot 2024-03-28 101320
Screenshot 2024-03-28 101352

But while trying to Select a team from osm teams it fails with unauthorized, invalid token
There is osmteams_token in localstorage

Screenshot 2024-03-28 101443

On backend side the token are being fetched and on frontend stored on the use browser but the auth fails to https://mapping.team/api/my/teams

@willemarcel Can you provide me some insights what wrong over this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let organisations import OSM teams into Tasking Manager for better coordination on remote and field mapping
7 participants