Changes necessary in master codebase to achieve a totally isolated meridian plugin #15126

ajaest · 2019-10-03T11:37:16Z

This is just a stub.

I will properly comment all changes in this PR with references to the new BCG - meridian pluggable gear.

lib/tasks/db_maintenance.rake

gonzaloriestra

For now, it looks good to me. Nice refactor with the partials ❤️

But the important stuff is still pending, right? Btw, there's a failing test because of the new property in the presenter.

webpack/v4/webpack.base.config.js

webpack/v4/gearAwareResolver.js

webpack/v4/webpack.base.config.js

webpack/v4/gearAwareResolver.js

lib/build/files/webpack_files.js

~ Fully working gear overriding webpack plugin ~ Some changes to allow user role monkey-patching in account/profile views ~ Changed webpack build and build:static file discovery method to allow overriding files from gears ~ UserRoleIndicator in admin organization's list view comes from backend instead than being resolved in template using the :viewer attribute ~ split app/view.admin/organization_users/_form.html.erb into section snippets to allow easily overriding sections from private_gears ~ rake carto::add_test_users_to_organization prepends the organization name to the username to avoid too short automatic usernames used as passwords

webpack/v4/entryPoints.js

lib/build/files/webpack_files.js

webpack/v4/webpack.base.config.js

lib/assets/javascripts/dashboard/data/user-model.js

…views/account/account-form-view.js to be overriden by gears

lib/assets/javascripts/dashboard/views/account/account-form-view.js

.gitignore

ajaest · 2019-10-14T07:44:58Z

app/controllers/carto/api/user_presenter.rb

@@ -41,6 +41,7 @@ def to_poro
          quota_in_bytes:             @user.quota_in_bytes,
          table_count:                @user.table_count,
          viewer:                     @user.viewer?,
+          role_display:               @user.viewer? ? 'viewer' : 'builder',


Problem

In BCG we needed to create custom role names for users apart from classic viewer and builder.

Current situation

In some places in the builder, there exist a small box close to the user's name stating the user role, either VIEWER or BUILDER:

The way these boxes are rendered are repeated and pretty static. Here is an example:

cartodb/lib/assets/javascripts/dashboard/views/organization/organization-users/organization-user.tpl

Lines 12 to 18 in e56cc62

<h3 class="CDB-Text u-ellipsLongText CDB-Size-medium is-semibold u-rSpace" title="<%- username %>"><%- username %></h3>

<% if (isOwner) { %>

<span class="UserRoleIndicator UserRoleIndicator--filled is-green u-lSpace">OWNER</span>

<% } else if (isAdmin) { %>

<span class="UserRoleIndicator UserRoleIndicator--filled is-grey u-lSpace">ADMIN</span>

<% } %>

<span class="UserRoleIndicator u-altTextColor u-lSpace u-upperCase">

They depend exclusively on whether the viewer db attribute is true or false. We extended this logic to have more than two roles depending on other factors, so the existing implementation didn't allowed us to extend the number of roles existing.

Solution

First, we made the API serve the role name as a string. I used the name role_display because the term role was already specified as class name in the original snippet (class="UserRoleIndicator") and display because there are examples of labels having this suffix here and there in the application.

Once done this in the API, we had to change the respective JS models to provide a getters being feed from the new API attribute:

lib/assets/javascripts/dashboard/views/profile/profile-form/profile-form-view.js

...vascripts/dashboard/views/organization/organization-users/organization-users-list-view.js

lib/assets/javascripts/dashboard/data/user-model.js

Then, the templates:

cartodb/lib/assets/javascripts/dashboard/views/profile/profile-form/profile-form.tpl

Lines 23 to 43 in e56cc62

<% if (role == 'viewer') { %>

<div>

<span class="UserRoleIndicator Viewer CDB-Text CDB-Size-small is-semibold u-altTextColor"><%= _t('profile.views.form.viewer') %></span>

<% if (hasOrganization) { %>

<a href="mailto:<%= orgDisplayEmail %>"><%= _t('profile.views.form.become_builder') %></a>

<% } %>

</div>

<p class="CDB-Text CDB-Size-small u-altTextColor u-tSpace"><%= _t('profile.views.form.read_only') %></p>

<% } else if (role == 'builder') { %>

<span class="UserRoleIndicator Builder CDB-Text CDB-Size-small is-semibold u-altTextColor"><%= _t('profile.views.form.builder') %></span>

<p class="CDB-Text CDB-Size-small u-altTextColor u-tSpace"><%= _t('profile.views.form.write_access') %></p>

<% } else { %>

<span class="UserRoleIndicator Builder CDB-Text CDB-Size-small is-semibold u-altTextColor"><%= role %></span>

<p class="CDB-Text CDB-Size-small u-altTextColor u-tSpace">

<% if (isViewer) { %>

<%= _t('profile.views.form.read_only') %>

<% } else { %>

<%= _t('profile.views.form.write_access') %>

<% } %>

</p>

<% } %>

cartodb/lib/assets/javascripts/dashboard/views/organization/organization-users/organization-user.tpl

Lines 18 to 20 in cf844d0

<span class="UserRoleIndicator u-altTextColor u-lSpace u-upperCase">

<%- role %>

</span>

Finally, it would be easy to make the FE display the custom roles by monkey-patching the serializers in charge of generating the user payload from the user model:

https://github.com/CartoDB/cartodb-bcg/blob/e912b7f2f6ad2afc9a70eff4ad494639ff0b6bb0/gear/app/overrides/carto_api.rb#L1-L26

ajaest · 2019-10-14T07:45:51Z

app/controllers/carto/api/user_presenter.rb

@@ -123,6 +124,7 @@ def data(options = {})
          regular_api_key_quota: @user.regular_api_key_quota,
          table_count: @user.table_count,
          viewer: @user.viewer?,
+          role_display: @user.viewer? ? 'viewer' : 'builder',


See https://github.com/CartoDB/cartodb/pull/15126/files#r334356417

ajaest · 2019-10-14T07:46:58Z

lib/assets/javascripts/dashboard/data/user-model.js

@@ -80,6 +80,10 @@ const UserModel = Backbone.Model.extend({
    return !this.isViewer();
  },

+  role: function () {


See https://github.com/CartoDB/cartodb/pull/15126/files#r334356417

ajaest · 2019-10-14T08:01:10Z

lib/assets/javascripts/dashboard/views/account/account-form-view.js

+        view: this
+      };
+
+      templateParams.accountFormExtension = accountFormExtensionTemplate(templateParams);


In order to be able to extend the user profile account form with custom fields, first we had to include a hook that may be overriden by gears. To do so, first we created an empty template which is required and included in the main template (see, accountFormExtensionTemplate, here, here and here).

If no gear overrides lib/assets/javascripts/dashboard/views/account/account-form-extension.tpl, this template would only generate an empty string and the account form would look as before these changes.

In BCG, we override that file to include our custom changes rather hacky but pragmatically:

https://github.com/CartoDB/cartodb-bcg/blob/e912b7f2f6ad2afc9a70eff4ad494639ff0b6bb0/gear/lib/assets/javascripts/dashboard/views/account/account-form-extension.tpl

We also needed to notify the patch that the render had finished successfully, reason why we included the onRenderFinished callback, which is patched in BCG as follows:

https://github.com/CartoDB/cartodb-bcg/blob/e912b7f2f6ad2afc9a70eff4ad494639ff0b6bb0/gear/lib/assets/javascripts/dashboard/views/account/account-form-view-extension.js#L29

ajaest · 2019-10-14T08:02:17Z

lib/assets/javascripts/dashboard/views/account/account-form-view.js

    }

    return this;
  },

+  onRenderFinished: function () {


See https://github.com/CartoDB/cartodb/pull/15126/files#r334361702

ajaest · 2019-10-14T08:02:23Z

lib/assets/javascripts/dashboard/views/account/account-form.tpl

@@ -39,6 +39,8 @@
    </div>
  <% } %>

+  <%= accountFormExtension %>


See https://github.com/CartoDB/cartodb/pull/15126/files#r334361702

ajaest · 2019-10-14T08:02:45Z

...assets/javascripts/dashboard/views/organization/organization-users/organization-user-view.js

@@ -18,6 +18,7 @@ module.exports = CoreView.extend({
        isOwner: this.options.isOwner,
        isAdmin: this.options.isAdmin,
        isViewer: this.options.isViewer,
+        role: this.options.role,


See https://github.com/CartoDB/cartodb/pull/15126/files#r334356417

ajaest · 2019-10-14T08:02:53Z

lib/assets/javascripts/dashboard/views/organization/organization-users/organization-user.tpl

-            <% } else { %>
-              BUILDER
-            <% } %>
+          <span class="UserRoleIndicator u-altTextColor u-lSpace u-upperCase">


See https://github.com/CartoDB/cartodb/pull/15126/files#r334356417

ajaest · 2019-10-14T08:03:05Z

.../javascripts/dashboard/views/organization/organization-users/organization-users-list-view.js

@@ -39,6 +39,7 @@ module.exports = CoreView.extend({
        isOwner: user.isOrgOwner(),
        isAdmin: user.isOrgAdmin(),
        isViewer: user.get('viewer'),
+        role: user.get('role_display'),


See https://github.com/CartoDB/cartodb/pull/15126/files#r334356417

ajaest · 2019-10-14T08:03:15Z

lib/assets/javascripts/dashboard/views/profile/profile-form/profile-form-view.js

@@ -78,6 +78,7 @@ module.exports = CoreView.extend({
        },
        isViewer: this._userModel.isViewer(),
        isInsideOrg: this._userModel.isInsideOrg(),
+        role: this._userModel.role(),


See https://github.com/CartoDB/cartodb/pull/15126/files#r334356417

ajaest · 2019-10-14T08:03:22Z

lib/assets/javascripts/dashboard/views/profile/profile-form/profile-form.tpl

@@ -20,17 +20,26 @@
        <label class="CDB-Text CDB-Size-medium is-semibold u-mainTextColor"><%= _t('profile.views.form.user_type') %></label>
      </div>
      <div class="FormAccount-rowData FormAccount-userRole">
-        <% if (isViewer) { %>
+        <% if (role == 'viewer') { %>


See https://github.com/CartoDB/cartodb/pull/15126/files#r334356417

alrocar · 2020-03-06T15:47:36Z

Thanks Victor, I guess we should ask someone out of this PR to do acceptance.

I'm summoning @ernesmb since he has been somehow involved. Would you be available for some acceptance tests next week?

ernesmb · 2020-03-09T08:10:11Z

yep! I'm not sure what I need to test though. These changes have been in Meridian (applied manually) for a few months and are working fine.

I guess for Builder the expected result of testing this changes is that the admin's UI works as expected?

alrocar · 2020-03-09T10:13:34Z

I guess for Builder the expected result of testing this changes is that the admin's UI works as expected?

That's it. Once we merge this, it's gonna be in our master branch so we need to make sure we didn't break anything.

I can deploy this to staging and give you some users for you to test. Let me know if that'll work for you and your availability (to deploy in staging just when you could test)

ernesmb · 2020-03-09T10:35:49Z

thanks @alrocar !

I could test tomorrow morning, does that work for you too?

ernesmb · 2020-03-16T16:14:08Z

hey, I couldn't test because of issues with VPN, then Superadmin access in Staging, etc..

could we have this redeployed in staging for testing? 🙏

gonzaloriestra · 2020-03-17T08:05:49Z

@ernesmb I've just deployed this to a dedicated instance in staging (during 1 week):

Regular user: meridian-plugin
Organization owner: meridian-plugin-org-admin
Organization user: meridian-plugin-org-1

I guess you should also install the private gear for the acceptance, but I'm not sure about the purpose of this PR... Please, let me know if you need help to do it.

ernesmb · 2020-03-17T11:31:31Z

thanks a lot @gonzaloriestra

the custom gear is already in production for BCG's onprem, and working fine.

the purpose of this test is making sure that it doesn't break anything in absence of the gear :)

ernesmb · 2020-03-17T11:59:56Z

so, after reviewing the parts that could be affected by this changes, the only thing I found is this alignment fault between the arrow and the user name in https://meridian-plugin-org.carto-staging.com/u/meridian-plugin-org-admin/organization/users/meridian-plugin-org-1/edit

I don't know if it's related to the changes, is there other organization admin account that we can use to compare?

VictorVelarde · 2020-03-17T12:44:09Z

I aggree with @gonzaloriestra about the testing needs. IMO there should be 2 areas:

a) testing that these 'adaptation' changes don't break anything on a common cartodb, WITHOUT gears
b) testing again that the MERIDIAN work is still fine with the current MASTER (it looks like the previous test was some months ago).

ernesmb · 2020-03-17T12:55:19Z

@VictorVelarde yes I agree on that.

Your point a) is what I'm trying to test now, please see my last comment about an arrow not being aligned with the username in https://meridian-plugin-org.carto-staging.com/u/meridian-plugin-org-admin/organization/users/meridian-plugin-org-1/edit

VictorVelarde · 2020-03-26T16:19:17Z

Hi @gonzaloriestra

I have talked to @ernesmb and, after talking to @ajaest, we have decided to move this forward.

Testing was made on staging to ensure this doesn't break current cdb, and in the future, more testing will be done to check with the gears in the on-prem env.

So, unless any other concern or extra check from backend side, I think we're ready to merge this in.

gonzaloriestra · 2020-03-27T07:07:53Z

Nice, thank you all for the effort! Let's go! 🚀

VictorVelarde · 2020-03-27T11:40:43Z

Already deployed to prod 🚀 !!

ajaest added the do not merge yet label Oct 3, 2019

ajaest requested a review from alrocar October 3, 2019 11:37

houndci-bot reviewed Oct 3, 2019

View reviewed changes

lib/tasks/db_maintenance.rake Show resolved Hide resolved

alrocar requested a review from gonzaloriestra October 3, 2019 16:33

gonzaloriestra reviewed Oct 4, 2019

View reviewed changes

houndci-bot reviewed Oct 4, 2019

View reviewed changes

houndci-bot reviewed Oct 7, 2019

View reviewed changes

ajaest changed the base branch from master to v4_27_1 October 7, 2019 14:16

ajaest force-pushed the v4.29.0-pluggable-private-gears branch from ae406c6 to cf844d0 Compare October 7, 2019 14:21

houndci-bot reviewed Oct 7, 2019

View reviewed changes

Luis Arce added 2 commits October 9, 2019 17:07

~ Extension to add tabs to the organization menu

93c4f5a

~ Added on finish event handler for lib/assets/javascripts/dashboard/…

e56cc62

…views/account/account-form-view.js to be overriden by gears

houndci-bot reviewed Oct 11, 2019

View reviewed changes

lib/assets/javascripts/dashboard/views/account/account-form-view.js Outdated Show resolved Hide resolved

ajaest commented Oct 14, 2019

View reviewed changes

.gitignore Show resolved Hide resolved

ajaest commented Oct 14, 2019

View reviewed changes

.gitignore Show resolved Hide resolved

ajaest commented Oct 14, 2019

View reviewed changes

Update user-model.js

20bd3de

ajaest commented Oct 14, 2019

View reviewed changes

Update account-form-view.js

072336c

ajaest commented Oct 14, 2019

View reviewed changes

Merge branch 'master' into v4.29.0-pluggable-private-gears

ab2fa32

alrocar approved these changes Mar 6, 2020

View reviewed changes

alrocar removed the waiting for frontend label Mar 6, 2020

Alberto Romeu added 2 commits March 10, 2020 10:47

merge master

45594bd

merge

6102057

alrocar assigned ernesmb Mar 10, 2020

Merge master

ea689aa

Merge branch 'master' into v4.29.0-pluggable-private-gears

930aa0c

VictorVelarde added 6 commits March 26, 2020 08:57

Merge branch 'master' into v4.29.0-pluggable-private-gears

28b592b

Fix group header style

ae47926

Merge branch 'master' into v4.29.0-pluggable-private-gears

cf62d58

Merge branch 'master' into v4.29.0-pluggable-private-gears

5a754ec

Revert temporary assets bump

417e748

Bump assets version

65ab183

Fix header style on users admin

9a90c00

VictorVelarde merged commit 49f11e5 into master Mar 27, 2020

VictorVelarde deleted the v4.29.0-pluggable-private-gears branch March 27, 2020 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes necessary in master codebase to achieve a totally isolated meridian plugin #15126

Changes necessary in master codebase to achieve a totally isolated meridian plugin #15126

ajaest commented Oct 3, 2019

gonzaloriestra left a comment

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

ajaest Oct 14, 2019

alrocar commented Mar 6, 2020

ernesmb commented Mar 9, 2020

alrocar commented Mar 9, 2020

ernesmb commented Mar 9, 2020

ernesmb commented Mar 16, 2020

gonzaloriestra commented Mar 17, 2020

ernesmb commented Mar 17, 2020

ernesmb commented Mar 17, 2020

VictorVelarde commented Mar 17, 2020

ernesmb commented Mar 17, 2020 •

edited

Loading

VictorVelarde commented Mar 26, 2020

gonzaloriestra commented Mar 27, 2020

VictorVelarde commented Mar 27, 2020

	<h3 class="CDB-Text u-ellipsLongText CDB-Size-medium is-semibold u-rSpace" title="<%- username %>"><%- username %></h3>
	<% if (isOwner) { %>
	<span class="UserRoleIndicator UserRoleIndicator--filled is-green u-lSpace">OWNER</span>
	<% } else if (isAdmin) { %>
	<span class="UserRoleIndicator UserRoleIndicator--filled is-grey u-lSpace">ADMIN</span>
	<% } %>
	<span class="UserRoleIndicator u-altTextColor u-lSpace u-upperCase">

	<% if (role == 'viewer') { %>
	<div>
	<span class="UserRoleIndicator Viewer CDB-Text CDB-Size-small is-semibold u-altTextColor"><%= _t('profile.views.form.viewer') %></span>
	<% if (hasOrganization) { %>
	<a href="mailto:<%= orgDisplayEmail %>"><%= _t('profile.views.form.become_builder') %></a>
	<% } %>
	</div>
	<p class="CDB-Text CDB-Size-small u-altTextColor u-tSpace"><%= _t('profile.views.form.read_only') %></p>
	<% } else if (role == 'builder') { %>
	<span class="UserRoleIndicator Builder CDB-Text CDB-Size-small is-semibold u-altTextColor"><%= _t('profile.views.form.builder') %></span>
	<p class="CDB-Text CDB-Size-small u-altTextColor u-tSpace"><%= _t('profile.views.form.write_access') %></p>
	<% } else { %>
	<span class="UserRoleIndicator Builder CDB-Text CDB-Size-small is-semibold u-altTextColor"><%= role %></span>
	<p class="CDB-Text CDB-Size-small u-altTextColor u-tSpace">
	<% if (isViewer) { %>
	<%= _t('profile.views.form.read_only') %>
	<% } else { %>
	<%= _t('profile.views.form.write_access') %>
	<% } %>
	</p>
	<% } %>

	<span class="UserRoleIndicator u-altTextColor u-lSpace u-upperCase">
	<%- role %>
	</span>

Changes necessary in master codebase to achieve a totally isolated meridian plugin #15126

Changes necessary in master codebase to achieve a totally isolated meridian plugin #15126

Conversation

ajaest commented Oct 3, 2019

gonzaloriestra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Problem

Current situation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar commented Mar 6, 2020

ernesmb commented Mar 9, 2020

alrocar commented Mar 9, 2020

ernesmb commented Mar 9, 2020

ernesmb commented Mar 16, 2020

gonzaloriestra commented Mar 17, 2020

ernesmb commented Mar 17, 2020

ernesmb commented Mar 17, 2020

VictorVelarde commented Mar 17, 2020

ernesmb commented Mar 17, 2020 • edited Loading

VictorVelarde commented Mar 26, 2020

gonzaloriestra commented Mar 27, 2020

VictorVelarde commented Mar 27, 2020

ernesmb commented Mar 17, 2020 •

edited

Loading