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

CARTO Builder activated notification always shown in account/profile pages #13691

Closed
javitonino opened this issue Mar 12, 2018 · 12 comments
Closed
Assignees

Comments

@javitonino
Copy link
Contributor

screenshot_20180312_164144

That notification is always shown in account/profile pages (but not in the dashboard), even though it has been marked as read:

dashboardNotifications = Object { builder_activated: true }

The problem is that, in this case frontend is checking for dashboard_notifications. There is a mix of both styles in both frontend and backend, but it's not always right.

I'd encourage to standardize in a single style, because it's crazy otherwise.

cc @ramiroaznar

@VictorVelarde
Copy link
Contributor

As @javitonino explained, the problem was misspelled variable names among front & back.

In both cases dashboardNotifications has to be renamed to dashboard_notifications (at /cartodb/app/views/admin/users/account.html.erb and /cartodb/app/views/admin/users/profile.html.erb)

We should have a look at other similar pages for consistency on variable naming and to avoid this kind of errors.

@VictorVelarde
Copy link
Contributor

This seems to be the same issue as #12091

@rubenmoya
Copy link
Contributor

In production! 🚀

@zingbot
Copy link

zingbot commented May 14, 2018

Nice! Thank you so much for removing this.

@alonsogarciapablo
Copy link
Contributor

It looks like this is still being displayed. @ramiroaznar just reported it:

image

@alonsogarciapablo
Copy link
Contributor

alonsogarciapablo commented Feb 14, 2019

@ramiroaznar We should look into the code to understand when this is currently being displayed so that we can decide wether to keep it in some cases or remote it forever and ever.

@jsanz
Copy link

jsanz commented Mar 25, 2019

@rjimenezda any update on this issue? Should we take it out of the RT kanban?

@jesusbotella
Copy link
Contributor

jesusbotella commented Mar 26, 2019

I've just checked this and the notification is shown in Builder, Dataset, Account, Profile, and API Keys page.

The notification has the same conditions in every page to be shown. It is checked that:

  • CARTO is hosted in carto.com
  • Has builder enabled
  • show_builder_activated_message variable is true in user
  • There are no dashboard notifications

Here's the code snippet of one of the pages:

if (!userModel.get('cartodb_com_hosted')) {
if (userModel.get('actions').builder_enabled && userModel.get('show_builder_activated_message') &&
_.isEmpty(window.dashboard_notifications)) {
const userNotificationModel = new UserNotificationModel(window.dashboard_notifications, {
key: 'dashboard',
configModel: configModel
});
const dashboardNotification = new UserNotificationView({
notification: userNotificationModel
});
window.dashboardNotification = dashboardNotification;
}
}

Seems like conditions are the same for all pages. We can close this issue and open a new one if we want to remove that notification.

@alonsogarciapablo
Copy link
Contributor

Let's 🔥🔥🔥 the notification!

@javitonino
Copy link
Contributor Author

Let's firefirefire the notification!

Please, if you do that, also remove the code from backend (the show_builder_activated_message part)

@jesusbotella
Copy link
Contributor

Fired, merged and deployed! 🔥

@alonsogarciapablo
Copy link
Contributor

Great @jesusbotella!

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

No branches or pull requests