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

SQLViewData is not read only when filtering #3812

Merged
merged 12 commits into from
Jun 9, 2015

Conversation

alonsogarciapablo
Copy link
Contributor

Fixes #152. Basically: you will now be able to edit cells while filtering, but not when a sql query has been applied, and active filters are updated accordingly :)

@@ -99,10 +95,10 @@ cdb.admin.SQLViewData = cdb.admin.CartoDBTableData.extend({
});
},

// TODO: Is this being used?
filterColumn: function(columnName, tableName, filter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found two references to this method:

@xavijam do you think I can remove it?

@saleiva
Copy link
Contributor

saleiva commented May 27, 2015

THIS IS FUCKING GOOD

@xavijam
Copy link
Contributor

xavijam commented May 28, 2015

@javisantana, please, could you take care of this PR?

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@alonsogarciapablo
Copy link
Contributor Author

@javisantana This is ready for review again. There was some hidden work here: filters were not being re-rendered correctly when the data was saved because:

  • data:saved was not being triggered correctly.
  • histograms were not prepared to respond to "expansion" of lower and upper values.

Thx to @javierarce for helping me out with the histogram issue!

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@@ -1,5 +1,13 @@
describe('modules.Filters', function() {

var clone = function(obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not _.clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Thanks!

@alonsogarciapablo
Copy link
Contributor Author

Hey @javierarce, can you please take a look at this PR? Thanks!

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@javierarce
Copy link
Contributor

@alonsogarciapablo: code looks good to me 👍

// filter table by column
this.setSQL(this.filterColumnSQL(columnName, tableName, filter));
this.readOnly = false;
return this.sqlSource() !== 'filters';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing that we could check here to determine if a row is editable:

return !!this.getSQL().indexOf("SELECT * FROM " + this.table.get('id'))

This way, SQLViews (and cells) will be editable when filtering or when a SQL query is applied and all the columns are being selected.

cc/ @javisantana

alonsogarciapablo added a commit that referenced this pull request Jun 9, 2015
SQLViewData is not read only when filtering
@alonsogarciapablo alonsogarciapablo merged commit 10ce188 into master Jun 9, 2015
@alonsogarciapablo alonsogarciapablo deleted the 152-edit-data-while-filtering branch June 9, 2015 09:14
@saleiva
Copy link
Contributor

saleiva commented Jun 9, 2015

Should it work on the Map page too? I'm not able to edit my data in the map page (dataset view) when filtering on a category column...

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@alonsogarciapablo
Copy link
Contributor Author

Yes, it should work:

filtering

@saleiva
Copy link
Contributor

saleiva commented Jun 10, 2015

weird, now it works :) Thanks!

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

Successfully merging this pull request may close these issues.

You should be able to edit your data in this mode
6 participants