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

Group by Sender and Recipient #567

Merged
merged 11 commits into from Dec 3, 2020
Merged

Group by Sender and Recipient #567

merged 11 commits into from Dec 3, 2020

Conversation

michielderoos
Copy link
Contributor

@michielderoos michielderoos commented Dec 1, 2020

Describe the Pull Request

Adds ability to group by sender and recipient! Also adds ability to hide them!

Todo

  • Link relevant trello card or related issue to the pull request
  • Request review from relevant @person or team
  • QA your pull request. This includes running the app, login, testing changes etc.
  • Bump backend and/or frontend versions following Semver

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #567 (4e8d763) into master (313bbe0) will increase coverage by 0.03%.
The diff coverage is 40.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
+ Coverage   47.24%   47.27%   +0.03%     
==========================================
  Files         338      338              
  Lines       17527    17560      +33     
  Branches     1335     1345      +10     
==========================================
+ Hits         8280     8302      +22     
- Misses       9214     9225      +11     
  Partials       33       33              
Flag Coverage Δ
javascript 4.77% <0.00%> (-0.02%) ⬇️
python 70.46% <90.90%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pp/client/components/filterModule/FilterModule.jsx 0.00% <0.00%> (ø)
app/server/utils/metrics/postprocessing_actions.py 93.45% <71.42%> (-0.46%) ⬇️
app/server/utils/metrics/group.py 100.00% <100.00%> (+4.08%) ⬆️
app/server/utils/metrics/metrics_const.py 100.00% <100.00%> (ø)
config.py 82.89% <100.00%> (ø)
app/server/__init__.py 89.86% <0.00%> (+0.46%) ⬆️
app/server/utils/transfer_filter.py 91.08% <0.00%> (+5.94%) ⬆️
app/server/models/custom_attribute.py 100.00% <0.00%> (+6.45%) ⬆️

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 313bbe0...4e8d763. Read the comment docs.

@michielderoos
Copy link
Contributor Author

Copy link
Contributor

@tristanhcole tristanhcole left a comment

Choose a reason for hiding this comment

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

One small change on frontend, otherwise code lgtm and tested.

Don't have enough context/looked into it enough to understand group joining strategies well

Comment on lines 147 to 163
<OptGroup label={"Sender"}>
{senderGroups.map(group => {
return (
<Option key={group}>{allowedGroups[group]["name"]}</Option>
);
})}
</OptGroup>
}
if(recipientGroups)
{
<OptGroup label={"Recipient"}>
{recipientGroups.map(group => {
return (
<Option key={key}>
{toTitleCase(replaceUnderscores(key))}
</Option>
<Option key={group}>{allowedGroups[group]["name"]}</Option>
);
})
: null}
})}
</OptGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking a hint out of @enjeyw OptGroup PR, can we add showing the group name once selected. See example from filter.jsx

let subList = keys.map(key => {
      //Here we show the label without the group in the dropdown, but with the group once selected
      let label = replaceUnderscores(possibleFilters[key]["name"] || key);
      return (
        <Option key={key} label={label}>
          {label.replace(userGroup, "")}
        </Option>
      );
    });

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!

@@ -49,7 +69,11 @@ def build_query_group_by_with_join(self, query, query_object_model):
args = (grouped_query, self.custom_attribute_field_name)
else:
args = (grouped_query,)
return group_joining_strategies[query_object_model.__tablename__][self.group_object_model.__tablename__](*args)
if self.sender_or_recipient == 'recipient':
Copy link
Contributor

Choose a reason for hiding this comment

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

self.sender_or_recipient == 'recipient':
sorry coming into this code without context but this seems strange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context for this change is here, this is just extending the feature to groups from an existing feature for filters.

Copy link
Contributor

@tristanhcole tristanhcole left a comment

Choose a reason for hiding this comment

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

good to go!

@michielderoos michielderoos merged commit 711b6ee into master Dec 3, 2020
@michielderoos michielderoos deleted the group-tx-rx branch December 3, 2020 12:45
@michielderoos michielderoos restored the group-tx-rx branch December 4, 2020 15:07
enjeyw added a commit that referenced this pull request Dec 7, 2020
* master:
  feat: small improvements to accessibility across the web app (#562)
  Group by Sender and Recipient (#567)
  Enable multiple chains (#496)
  Silence loading spinner warning (#568)
  Custom attribute bugfix (#549)
  Antify Message Bar (#566)

# Conflicts:
#	config.py
#	eth_worker/eth_src/celery_app.py
#	eth_worker/eth_src/sql_persistence/interface.py
enjeyw added a commit that referenced this pull request Dec 15, 2020
* master: (28 commits)
  Adding blockchain_task_uuid index to worker messages (#551)
  Make location lookup look at existing users' coordinates as a last resort (#555)
  Add support for Commcare (#569)
  Improve external transaction detection (#570)
  Remove unused imports (#571)
  Celo integration (#535)
  feat: small improvements to accessibility across the web app (#562)
  Group by Sender and Recipient (#567)
  Enable multiple chains (#496)
  Silence loading spinner warning (#568)
  Custom attribute bugfix (#549)
  Antify Message Bar (#566)
  Stop retrying infinitely (#565)
  Only update BlockchainTransaction on the worker when we absolutely have to (#564)
  Make SynchronizedBlock be updated in batch instead of one giant update (#563)
  Hide Filters and Groups (#558)
  adding more colors to charts, and tweaking legends (#561)
  Chart Tweaks (#560)
  Recipients filter (#533)
  Dynamic transfer card loaded (#559)
  ...

# Conflicts:
#	app/server/api/transfer_card_api.py
#	app/test_app/functional/api/test_transfer_card_api.py
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.

None yet

2 participants