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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "No activity" message in Last Activities isn't shown sometimes #10385

Merged
merged 5 commits into from Jun 14, 2023

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Feb 14, 2023

馃帺 What? Why?

As per #10384 has been described that participatory spaces do not display any activity cards, nor the message there are no activities.
This Pr fixes this.

馃搶 Related Issues

Link your PR to an issue

Testing

  1. Go to last activities page
  2. Select a participatory space
  3. See there is no message
  4. Apply the patch
  5. refresh the page and see the improvement

鈾ワ笍 Thank you!

@alecslupu alecslupu added module: core type: fix PRs that implement a fix for a bug labels Feb 14, 2023
@alecslupu alecslupu marked this pull request as ready for review February 14, 2023 09:12
@Crashillo
Copy link
Member

Even though you can go forward with this, it's not a good solution. First off, you need to look over the cell uses to check whether the controller implements the announcement or not (for instance, I realized this bug because the controller did it but displayed nothing afterwards).
Secondly, let's say you have pagination, we're stuck in the same starter point: the controller cannot know how many items will show the cell, hence it cannot handle the pagination properly.

The bad approach here is the fact of having an "activities" (plural) cell. That's something it should be handled by the element (controller) who actually instantiates the activity (singular) cell. But let's say that's ok and we need the plural cell, then, what's wrong done in the cell logic is that visible_for is hiding some items, i.e. it's manipulating data, and that's breaks the rule of data-agnostics components: a component should not care about an element is this or that, but, given an input of N elements, returns an output of N elements.

Or you have a very complex component, with ability to hide/show items, render announcements, paginators, results_per_page components, etc... or you keep atomic components, and you use them as small pills wherever you want to do it (in the controllers)

@alecslupu
Copy link
Contributor Author

Even though you can go forward with this, it's not a good solution. First off, you need to look over the cell uses to check whether the controller implements the announcement or not (for instance, I realized this bug because the controller did it but displayed nothing afterwards). Secondly, let's say you have pagination, we're stuck in the same starter point: the controller cannot know how many items will show the cell, hence it cannot handle the pagination properly.

The bad approach here is the fact of having an "activities" (plural) cell. That's something it should be handled by the element (controller) who actually instantiates the activity (singular) cell. But let's say that's ok and we need the plural cell, then, what's wrong done in the cell logic is that visible_for is hiding some items, i.e. it's manipulating data, and that's breaks the rule of data-agnostics components: a component should not care about an element is this or that, but, given an input of N elements, returns an output of N elements.

Or you have a very complex component, with ability to hide/show items, render announcements, paginators, results_per_page components, etc... or you keep atomic components, and you use them as small pills wherever you want to do it (in the controllers)

I think this "activities" cell, is not really needed in the way is being used.
It would be required a greater effort to remove the queries from the cells, but considering the redesign process, i think is not a good moment to start making this kind of big changes.

@alecslupu
Copy link
Contributor Author

I have searched in the application for any usage of the activity cell, and i have found several places. and i think the UserTimelineCell is not being used.

Also, considering the data aggregation, that is required in order to determine whether a resource needs or does not need to be displayed, I think the fix is not so easy. In order to implement any viable pagination to this we would need to add an additional parameter, like "last Id" to store the last displayed activity id, so that we could manually the pagination, by injecting to the query a "where id < $lastID", then hydrate and construct the needed params for the next page.

I see few possible solutions:

  1. remove the last_activity concept
  2. Implement some other kind of query builder
  3. keep it as it is for the moment.
  4. Remove the spaces from the list.
  5. Write a custom sql that would aggregate almost the entire DB in a single db, like Activity_log, joins resources, joins moderations, joins spaces, components etc ...

@andreslucena andreslucena changed the title Fix: "No activity" message in Last Activities isn't shown sometimes Fix "No activity" message in Last Activities isn't shown sometimes Feb 15, 2023
@alecslupu alecslupu force-pushed the fix/10384 branch 6 times, most recently from 0f0bb75 to efd7900 Compare June 5, 2023 09:50
@alecslupu alecslupu requested a review from a team June 5, 2023 09:52
@alecslupu alecslupu added this to Pending review from Product in Maintainers via automation Jun 5, 2023
@andreslucena andreslucena self-assigned this Jun 7, 2023
@andreslucena
Copy link
Member

FYI, I've removed the unused section in the template

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I tried it locally and it works on my machine.

I have a couple suggestions, and also some doubts regarding the history of this PR. I don't know if all the doubts commented by @Crashillo were actually taken into account, as the git commit history isn't visible, with that many push forces. Can you chime in @Crashillo? Are you happy with the current approach?

decidim-core/spec/system/last_activity_spec.rb Outdated Show resolved Hide resolved
@Crashillo
Copy link
Member

Reviewing the code, @andreslucena, I think this a better approach since you have move that filtering logic to the query layer (that's separation of concerns 馃憤) In that way, the cell will receive and display the same amount of items, that was the purpose. Nice!

@alecslupu
Copy link
Contributor Author

@andreslucena now the PR seems to be ready now :)

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

LGTM 馃憤馃徑

@andreslucena andreslucena merged commit 993025e into develop Jun 14, 2023
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core type: fix PRs that implement a fix for a bug
Projects
Development

Successfully merging this pull request may close these issues.

"No activity" message in Last Activities isn't shown sometimes
3 participants