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

Columns dialog clean attribute ids before send it to GridAttributes operation #476

Merged
merged 5 commits into from
Apr 15, 2020
Merged

Columns dialog clean attribute ids before send it to GridAttributes operation #476

merged 5 commits into from
Apr 15, 2020

Conversation

gabmartinez
Copy link
Contributor

@gabmartinez gabmartinez commented Apr 11, 2020

I want to merge this change because it resolves the issue that was sending invalid ids to GridAttributes operation and it was getting an error from the API and frozen the dialog to add/remove columns.

Fix #478

Screenshots

Sending Invalid ids before the changes

Sending valid ids after the changes

Pull Request Checklist

  1. All visible strings are translated with proper context.
  2. All data-formatting is locale-aware (dates, numbers, and so on).
  3. Translated strings are extracted.
  4. Number of API calls is optimized.
  5. The changes are tested.
  6. Type definitions are up to date.
  7. Changes are mentioned in the changelog.

@codecov
Copy link

codecov bot commented Apr 11, 2020

Codecov Report

Merging #476 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #476   +/-   ##
=======================================
  Coverage   72.39%   72.39%           
=======================================
  Files         658      658           
  Lines        7440     7440           
  Branches     1227     1227           
=======================================
  Hits         5386     5386           
  Misses       1819     1819           
  Partials      235      235           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01d9545...bf3a0b4. Read the comment docs.

@dominik-zeglen
Copy link
Contributor

I have no idea what problem this PR solves. Would you mind explaining it?

@gabmartinez
Copy link
Contributor Author

gabmartinez commented Apr 14, 2020

I have no idea what problem this PR solves. Would you mind explaining it?

@dominik-zeglen After the PR 5460 the API is expecting valid IDs in the filter otherwise it will raise a GraphQLError. In the Dashboard, the feature to add more columns in the list of products was not working because we were sending invalid IDs like attribute:QXR0cmlidXRlOjE0 or isPublished and getting errors and it was not updating the list. So, now we are cleaning those values to send only valid IDs to the API.

I updated the description of the PR, the images had an incorrect title.

@gabmartinez gabmartinez changed the title Filter column attribute ids before send it to GridAttributes operation Filter column attribute clean ids before send it to GridAttributes operation Apr 14, 2020
@gabmartinez gabmartinez changed the title Filter column attribute clean ids before send it to GridAttributes operation Columns dialog clean attribute ids before send it to GridAttributes operation Apr 14, 2020
Comment on lines 222 to 229
const columnIdsFilter = columns => {
columns = columns.filter(column => isAttributeColumnValue(column));
columns.forEach(
(attribute, index, arr) =>
(arr[index] = getAttributeIdFromColumnValue(attribute))
);
return columns;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be shortened to

  function filterColumnIds(columns: ProductListColumns[]) {
    return columns.filter(isAttributeColumnValue).map(getAttributeIdFromColumnValue)
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, working on it. Thanks

Copy link
Contributor Author

@gabmartinez gabmartinez Apr 15, 2020

Choose a reason for hiding this comment

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

Also, we could move it to utils in the component to be able to reuse it in other places as we need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, it's specific for the available query in that view. I updated the description to include issue #478 created today.

Done, please review it again and thank you for the feedback :)

@dominik-zeglen dominik-zeglen merged commit ffced45 into saleor:master Apr 15, 2020
@gabmartinez gabmartinez deleted the fix/sending-invalid-ids-to-grid-attributes-request branch August 9, 2020 16:08
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.

Invalid query data sent in product list
3 participants