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

Improving UX by hiding editing buttons for readers of a project #3682

Merged
merged 23 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
36b2f39
wip - reader and guests cant edit
VitorVieiraZ Nov 18, 2024
8ecf262
in progress
VitorVieiraZ Nov 21, 2024
b3ded65
in progress
VitorVieiraZ Nov 22, 2024
5da5eca
updating project role on opening a project from active project and ch…
VitorVieiraZ Nov 27, 2024
7a01cc6
updating role every time we open a project + visibility condition on …
VitorVieiraZ Nov 28, 2024
eabfc85
in progress
VitorVieiraZ Dec 9, 2024
6596705
ensuring user role is cached/updated each time we open a project
VitorVieiraZ Dec 10, 2024
8a92bff
cleaning code
VitorVieiraZ Dec 10, 2024
193574d
merging master to branch
VitorVieiraZ Dec 10, 2024
493bb18
Merge branch 'master' into enhancementUiUx/readerCantEdit
VitorVieiraZ Jan 10, 2025
4a6deda
post review changes
VitorVieiraZ Jan 13, 2025
e0f23b2
tests and removing truncate
VitorVieiraZ Jan 13, 2025
9cd5999
decoupling mergin api from activeProject and rebuilding connections
VitorVieiraZ Jan 15, 2025
713b347
final adjustments
VitorVieiraZ Jan 18, 2025
3b94135
Merge branch 'master' into enhancementUiUx/readerCantEdit
VitorVieiraZ Jan 18, 2025
48d244f
replaceValueInJson tests
VitorVieiraZ Jan 20, 2025
585412e
post review updates
VitorVieiraZ Jan 21, 2025
709b3fc
merging master
VitorVieiraZ Jan 21, 2025
97888cb
code layout adjustment
VitorVieiraZ Jan 21, 2025
8e4a046
small fix
VitorVieiraZ Jan 21, 2025
0b28d3b
include merginprojectmetadata in activeproject
VitorVieiraZ Jan 21, 2025
49b2a6a
updating tests
VitorVieiraZ Jan 22, 2025
721c762
fixing tests
VitorVieiraZ Jan 22, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions app/activeproject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,15 @@ ActiveProject::ActiveProject( AppSettings &appSettings
, ActiveLayer &activeLayer
, LayersProxyModel &recordingLayerPM
, LocalProjectsManager &localProjectsManager
, MerginApi *merginApi
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved
, QObject *parent ) :

QObject( parent )
, mAppSettings( appSettings )
, mActiveLayer( activeLayer )
, mRecordingLayerPM( recordingLayerPM )
, mLocalProjectsManager( localProjectsManager )
, mMerginApi( merginApi )
, mProjectLoadingLog( "" )
{
// we used to have our own QgsProject instance, but unfortunately few pieces of qgis_core
Expand Down Expand Up @@ -74,6 +76,24 @@ ActiveProject::ActiveProject( AppSettings &appSettings
setAutosyncEnabled( mAppSettings.autosyncAllowed() );

QObject::connect( &mAppSettings, &AppSettings::autosyncAllowedChanged, this, &ActiveProject::setAutosyncEnabled );

QObject::connect(
mMerginApi,
&MerginApi::projectMetadataRoleUpdated,
this, [this]( const QString & projectFullName, const QString & role )
{
if ( projectFullName == this->projectFullName() )
{
setProjectRole( role );
}
} );

QObject::connect(
mMerginApi,
&MerginApi::authChanged,
this,
&ActiveProject::updateUserRoleInActiveProject
);
}

ActiveProject::~ActiveProject() = default;
Expand Down Expand Up @@ -188,6 +208,7 @@ bool ActiveProject::forceLoad( const QString &filePath, bool force )
updateRecordingLayers();
updateActiveLayer();
updateMapSettingsLayers();
updateUserRoleInActiveProject();

emit localProjectChanged( mLocalProject );
emit projectReloaded( mQgsProject );
Expand Down Expand Up @@ -553,3 +574,24 @@ bool ActiveProject::positionTrackingSupported() const

return mQgsProject->readBoolEntry( QStringLiteral( "Mergin" ), QStringLiteral( "PositionTracking/Enabled" ), false );
}

QString ActiveProject::projectRole() const
{
return mProjectRole;
}

void ActiveProject::setProjectRole( const QString &role )
{
if ( mProjectRole != role )
{
mProjectRole = role;

emit projectRoleChanged();
}
}

void ActiveProject::updateUserRoleInActiveProject()
{
// update user's role each time a project is opened or auth changes, following #3174
mMerginApi->updateProjectMetadataRole( projectFullName() );
}
19 changes: 19 additions & 0 deletions app/activeproject.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "localprojectsmanager.h"
#include "autosynccontroller.h"
#include "inputmapsettings.h"
#include "merginapi.h"
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved

/**
* \brief The ActiveProject class can load a QGIS project and holds its data.
Expand All @@ -33,6 +34,7 @@ class ActiveProject: public QObject
Q_PROPERTY( QgsProject *qgsProject READ qgsProject NOTIFY qgsProjectChanged ) // QgsProject instance of active project, never changes
Q_PROPERTY( AutosyncController *autosyncController READ autosyncController NOTIFY autosyncControllerChanged )
Q_PROPERTY( InputMapSettings *mapSettings READ mapSettings WRITE setMapSettings NOTIFY mapSettingsChanged )
Q_PROPERTY( QString projectRole READ projectRole WRITE setProjectRole NOTIFY projectRoleChanged )

Q_PROPERTY( QString mapTheme READ mapTheme WRITE setMapTheme NOTIFY mapThemeChanged )
Q_PROPERTY( bool positionTrackingSupported READ positionTrackingSupported NOTIFY positionTrackingSupportedChanged )
Expand All @@ -43,6 +45,7 @@ class ActiveProject: public QObject
, ActiveLayer &activeLayer
, LayersProxyModel &recordingLayerPM
, LocalProjectsManager &localProjectsManager
, MerginApi *mMerginApi
, QObject *parent = nullptr );

virtual ~ActiveProject();
Expand Down Expand Up @@ -118,6 +121,18 @@ class ActiveProject: public QObject

bool positionTrackingSupported() const;

/**
* Returns role/permission level of current user for this project
*/
Q_INVOKABLE QString projectRole() const;

void setProjectRole( const QString &role );

/**
* Calls Mergin API to update current project’s role
*/
Q_INVOKABLE void updateUserRoleInActiveProject();
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved

signals:
void qgsProjectChanged();
void localProjectChanged( LocalProject project );
Expand Down Expand Up @@ -145,6 +160,8 @@ class ActiveProject: public QObject
// Emited when the app (UI) should show tracking because there is a running tracking service
void startPositionTracking();

void projectRoleChanged();

public slots:
// Reloads project if current project path matches given path (its the same project)
bool reloadProject( QString projectDir );
Expand Down Expand Up @@ -182,10 +199,12 @@ class ActiveProject: public QObject
LayersProxyModel &mRecordingLayerPM;
LocalProjectsManager &mLocalProjectsManager;
InputMapSettings *mMapSettings = nullptr;
MerginApi *mMerginApi = nullptr;
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved

std::unique_ptr<AutosyncController> mAutosyncController;

QString mProjectLoadingLog;
QString mProjectRole;

/**
* Reloads project.
Expand Down
2 changes: 1 addition & 1 deletion app/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ int main( int argc, char *argv[] )
LayersProxyModel recordingLpm( &lm, LayerModelTypes::ActiveLayerSelection );

ActiveLayer al;
ActiveProject activeProject( as, al, recordingLpm, localProjectsManager );
ActiveProject activeProject( as, al, recordingLpm, localProjectsManager, ma.get() );
std::unique_ptr<VariablesManager> vm( new VariablesManager( ma.get() ) );
vm->registerInputExpressionFunctions();

Expand Down
4 changes: 2 additions & 2 deletions app/qml/form/MMFormPage.qml
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ Page {

footer: MMComponents.MMToolbar {

visible: !root.layerIsReadOnly
visible: !root.layerIsReadOnly && __activeProject.projectRole !== "reader"

ObjectModel {
id: readStateButtons
Expand Down Expand Up @@ -231,7 +231,7 @@ Page {
id: editGeometry
text: qsTr( "Edit geometry" )
iconSource: __style.editIcon
visible: root.layerIsSpatial
visible: root.layerIsSpatial && __activeProject.projectRole !== "reader"
onClicked: root.editGeometryRequested( root.controller.featureLayerPair )
}
}
Expand Down
2 changes: 1 addition & 1 deletion app/qml/form/MMPreviewDrawer.qml
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ Item {
property bool isHTMLType: root.controller.type === MM.AttributePreviewController.HTML
property bool isEmptyType: root.controller.type === MM.AttributePreviewController.Empty

property bool showEditButton: !root.layerIsReadOnly
property bool showEditButton: !root.layerIsReadOnly && __activeProject.projectRole !== "reader"
property bool showStakeoutButton: __inputUtils.isPointLayerFeature( controller.featureLayerPair )
property bool showButtons: showEditButton || showStakeoutButton

Expand Down
1 change: 1 addition & 0 deletions app/qml/form/components/MMFeaturesListPageDrawer.qml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ Drawer {
}

text: qsTr( "Add feature" )
visible: __activeProject.projectRole !== "reader"

onClicked: root.buttonClicked()
}
Expand Down
2 changes: 1 addition & 1 deletion app/qml/layers/MMFeaturesListPage.qml
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ MMComponents.MMPage {
anchors.bottom: parent.bottom
anchors.bottomMargin: root.hasToolbar ? __style.margin20 : ( __style.safeAreaBottom + __style.margin8 )

visible: __inputUtils.isNoGeometryLayer( root.selectedLayer ) && !root.layerIsReadOnly
visible: __inputUtils.isNoGeometryLayer( root.selectedLayer ) && !root.layerIsReadOnly && __activeProject.projectRole !== "reader"

text: qsTr("Add feature")

Expand Down
1 change: 1 addition & 0 deletions app/qml/main.qml
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ ApplicationWindow {
MMToolbarButton {
text: qsTr("Add")
iconSource: __style.addIcon
visible: __activeProject.projectRole !== "reader"
onClicked: {
if ( __recordingLayersModel.rowCount() > 0 ) {
stateManager.state = "map"
Expand Down
6 changes: 3 additions & 3 deletions app/test/testactiveproject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ void TestActiveProject::testProjectValidations()
ActiveLayer al;
LayersModel lm;
LayersProxyModel lpm( &lm, LayerModelTypes::ActiveLayerSelection );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager() );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager(), mApi );

QSignalSpy spyReportIssues( &activeProject, &ActiveProject::reportIssue );
QSignalSpy spyErrorsFound( &activeProject, &ActiveProject::loadingErrorFound );
Expand All @@ -66,7 +66,7 @@ void TestActiveProject::testProjectLoadFailure()
ActiveLayer al;
LayersModel lm;
LayersProxyModel lpm( &lm, LayerModelTypes::ActiveLayerSelection );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager() );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager(), mApi );

mApi->localProjectsManager().addLocalProject( projectdir, projectname );

Expand All @@ -88,7 +88,7 @@ void TestActiveProject::testPositionTrackingFlag()
ActiveLayer al;
LayersModel lm;
LayersProxyModel lpm( &lm, LayerModelTypes::ActiveLayerSelection );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager() );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager(), mApi );

// project "planes" - tracking not enabled
QString projectDir = TestUtils::testDataDir() + "/planes/";
Expand Down
30 changes: 29 additions & 1 deletion app/test/testmerginapi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2369,7 +2369,7 @@ void TestMerginApi::testAutosync()

MapThemesModel mtm; AppSettings as; ActiveLayer al;
LayersModel lm; LayersProxyModel lpm( &lm, LayerModelTypes::ActiveLayerSelection );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager() );
ActiveProject activeProject( as, al, lpm, mApi->localProjectsManager(), mApi );

mApi->localProjectsManager().addLocalProject( projectdir, projectname );

Expand Down Expand Up @@ -2942,3 +2942,31 @@ void TestMerginApi::testParseVersion()
QCOMPARE( major, 2024 );
QCOMPARE( minor, 4 );
}

void TestMerginApi::testUpdateProjectMetadataRole()
{
QString projectName = "testUpdateProjectMetadataRole";

createRemoteProject( mApiExtra, mWorkspaceName, projectName, mTestDataPath + "/" + TEST_PROJECT_NAME + "/" );
downloadRemoteProject( mApi, mWorkspaceName, projectName );

LocalProject projectInfo = mApi->localProjectsManager().projectFromMerginName( mWorkspaceName, projectName );
QVERIFY( projectInfo.isValid() );
QString projectDir = projectInfo.projectDir;

// Test 1: Initial role should be 'owner'
QString cachedRole = mApi->getCachedProjectRole( MerginApi::getFullProjectName( mWorkspaceName, projectName ) );
QCOMPARE( cachedRole, QString( "owner" ) );

// Test 2: Update cached role to 'reader'
QString newRole = "reader";
bool updateSuccess = mApi->updateCachedProjectRole( MerginApi::getFullProjectName( mWorkspaceName, projectName ), newRole );
QVERIFY( updateSuccess );

// Verify role was updated in cache
cachedRole = mApi->getCachedProjectRole( MerginApi::getFullProjectName( mWorkspaceName, projectName ) );
QCOMPARE( cachedRole, newRole );
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved

VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved
// Clean up
deleteRemoteProjectNow( mApi, mWorkspaceName, projectName );
}
1 change: 1 addition & 0 deletions app/test/testmerginapi.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class TestMerginApi: public QObject
void testSynchronizationViaManager();
void testAutosync();
void testAutosyncFailure();
void testUpdateProjectMetadataRole();

void testRegisterAndDelete();
void testCreateWorkspace();
Expand Down
107 changes: 107 additions & 0 deletions core/merginapi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3449,6 +3449,45 @@ bool MerginApi::writeData( const QByteArray &data, const QString &path )
return true;
}

bool MerginApi::updateCachedProjectRole( const QString &projectFullName, const QString &newRole )
{
LocalProject project = mLocalProjects.projectFromMerginName( projectFullName );
if ( !project.isValid() )
{
return false;
}

QString metadataPath = project.projectDir + "/" + sMetadataFile;

QFile file( metadataPath );
if ( !file.open( QIODevice::ReadOnly ) )
{
return false;
}

QByteArray data = file.readAll();
file.close();

QJsonDocument doc = QJsonDocument::fromJson( data );
if ( !doc.isObject() )
{
return false;
}

QJsonObject obj = doc.object();
obj["role"] = newRole;
doc.setObject( obj );

if ( !file.open( QIODevice::WriteOnly ) )
{
return false;
}

bool success = ( file.write( doc.toJson() ) != -1 );
file.close();
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved

return success;
}

void MerginApi::createPathIfNotExists( const QString &filePath )
{
Expand Down Expand Up @@ -3945,3 +3984,71 @@ DownloadQueueItem::DownloadQueueItem( const QString &fp, qint64 s, int v, qint64
tempFileName = CoreUtils::uuidWithoutBraces( QUuid::createUuid() );
}

void MerginApi::updateProjectMetadataRole( const QString &projectFullName )
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved
{
if ( projectFullName.isEmpty() )
{
return;
}

QString cachedRole = MerginApi::getCachedProjectRole( projectFullName );

QNetworkReply *reply = getProjectInfo( projectFullName );
if ( !reply )
{
emit projectMetadataRoleUpdated( projectFullName, cachedRole );
VitorVieiraZ marked this conversation as resolved.
Show resolved Hide resolved
return;
}

reply->request().setAttribute( static_cast<QNetworkRequest::Attribute>( AttrProjectFullName ), projectFullName );
connect( reply, &QNetworkReply::finished, this, &MerginApi::updateProjectMetadataRoleReplyFinished );
}

void MerginApi::updateProjectMetadataRoleReplyFinished()
{
QNetworkReply *r = qobject_cast<QNetworkReply *>( sender() );
Q_ASSERT( r );

QString projectFullName = r->request().attribute( static_cast<QNetworkRequest::Attribute>( AttrProjectFullName ) ).toString();
QString cachedRole = MerginApi::getCachedProjectRole( projectFullName );

if ( r->error() == QNetworkReply::NoError )
{
QByteArray data = r->readAll();
MerginProjectMetadata serverProject = MerginProjectMetadata::fromJson( data );
QString role = serverProject.role;

if ( role != cachedRole )
{
if ( updateCachedProjectRole( projectFullName, role ) )
{
emit projectMetadataRoleUpdated( projectFullName, role );
}
else
{
CoreUtils::log( "metadata", QString( "Failed to update cached role for project %1" ).arg( projectFullName ) );
}
}
}
else
{
emit projectMetadataRoleUpdated( projectFullName, cachedRole );
}

r->deleteLater();
}

QString MerginApi::getCachedProjectRole( const QString &projectFullName ) const
{
if ( projectFullName.isEmpty() )
return QString();

QString projectDir = mLocalProjects.projectFromMerginName( projectFullName ).projectDir;

if ( projectDir.isEmpty() )
return QString();

MerginProjectMetadata cachedProjectMetadata = MerginProjectMetadata::fromCachedJson( projectDir + "/" + sMetadataFile );

return cachedProjectMetadata.role;
}
Loading
Loading