OpenSourcePolitics / dev

On this repo OSP list the features for Decidim it can outsource to other companies and developers familiar with the framework.
1 stars 0 forks source link

Finish PR 5500 initiative signature gauge #36

Open virgile-dev opened 4 years ago

virgile-dev commented 4 years ago

We've started the PR with Codegram but didn't manage to finish it properly. Here is a comment on the PR with the remaining tasks.

The idea is to make a PR on this branch with the remaining changes.

ace commented 4 years ago

Hi @virgile-dev. I think I need a bit more detail about the remaining tasks.

Thanks!

virgile-dev commented 4 years ago

Hello @ace Here are my answers :

Check that events issued from initiatives are working properly. Are there any specifics about what to check or which events may be affected?

Check that the edge cases defined here work well. As far as I understand the linked doc, the main issue is having scopes below the level of detail in the authorization. In that case the signature is applied to all child scopes since we can't identify a specific one. Is that right?

Display a warning in the admin if an initiative does not have a global scope and only global scope config flag is activated on the initiative type. Not sure I understand this use case. I mean, is it possible to get to that situation? If that flag is activated, is it possible to create an initiative without a global scope? Maybe we should enforce that? Do you have a sample of how to display the warning?

Fix the initiative admin form when the scope is global. What's the issue there? Any kind of process error or just a visual fix?


scope global

Apply design in mockup for signature gauges. Is this related to the bottom margin issue you mentioned in that PR or is there anything else to be done? Not sure how the 'see all' link (mentioned in #9) should work. Does it 'unfold' the rest of the data or is it a link to a separate page (which one?).

ace commented 4 years ago

Thanks @virgile-dev Just one thing, do you have any example of how that warning in the admin should look like?

virgile-dev commented 4 years ago

@ace which warning are we talking about ? This one ? I'm not even sure it needs a warning as it should be compatible. Don't you.

I’m guessing this is more of compatibility issue. If there were already initiatives and you activate the global scope only flag what happens. IMO if you created an initiative with the child scope only the child scope gauge should be display. When it has a global scope then you display the child scope jauge if « child scope signature » flag is enabled. 


Warning display the same on the front end and in the back end.

Capture d’écran 2020-05-25 à 15 33 48
ace commented 4 years ago

Yes, that one. Not sure if it's needed, to be honest. If it is OK to be in such a state, does the warning provide any help or anything to the admin? I tend to think it does not, so maybe it is not needed at all. So, if you are OK with that, I'll leave it for now.

virgile-dev commented 4 years ago

I'm ok with no warning at all @ace

ace commented 4 years ago

I'll be keeping Issues I find and whether I've fixed them in this comment, so they can be checked quickly.

[x] Migrations fail (code updating the existing votes is wrong) [x] Admin editing initiative with offline votes crashes, I18n translation issue (using scope as interpolation key) [x] Offline votes saved from admin form as string arrays instead of integers and with no total. Example: online_votes => {"1"=>1, "total"=>1}; offline_votes => {"1"=>["15"], "2"=>["6"], "4"=>["4"]} [x] Required votes just adds up all the scopes required votes. For a parent (1000 votes required) and two children (500 votes required each), that means 2000 votes are needed, with no further check on where they apply. [x] Total votes just adds up all votes for all scopes. For a parent (1000 votes) and two children (600 and 400 votes), the total is 2000, again, with no check on where they apply. (Note that the two examples will result in an accepted initiative, 2000 = 2000, even though one child is not fulfilled). [x] Breaks sorting by signatures count in the admin [x] Sorting by signatures count fail when nils (almost always?) [x] Migrations fail because one of them removes a column that is used in a later migration

ace commented 4 years ago

@virgile-dev while I write the doubts we talked about (seeing some code that clarifies some of them), one more thing I'd need to clarify.

* We’d like to send notifications for child scope milestones too (same percentages : 25, 50, 75 and 100%). Notifications would go to authors, promotting committee, followers for all the milestones (25, 50, 75 and 100%). Admins should receive a notification only upon reaching 100%.

This is the content we are sending right now for those milestones for the author and others:

        milestone_completed:
          affected_user:
            email_intro: Your initiative %{resource_title} has achieved the %{percentage}% of signatures!
            email_outro: You have received this notification because you are the author of the initiative %{resource_title}.
            email_subject: New milestone completed!
            notification_title: Your <a href="%{resource_path}">%{resource_title}</a> initiative has achieved the %{percentage}% of signatures.
          follower:
            email_intro: The initiative %{resource_title} has achieved the %{percentage}% of signatures!
            email_outro: You have received this notification because you are following %{resource_title}. You can stop receiving notifications following the previous link.
            email_subject: New milestone completed!
            notification_title: The <a href="%{resource_path}">%{resource_title}</a> initiative has achieved the %{percentage}% of signatures.

How should we adapt those texts for the child scopes? Keeping them unique may lead to confusions. Same question for the admin messages for 100% in the other PR (https://github.com/decidim/decidim/pull/6098/files#diff-76dc41a8aba2e3fa43b6dfee29b71177).

One other thing to give a thought, not needed for now. With the process as it is now, we send a 100% milestone notification (and admin notification) for the parent scope when that scope gets enough votes, but that doesn't guarantee that the initiative is completed, because it may lack votes in child scopes. Maybe that will be confusing for the users. In fact all percentages for the parent scope may be missleading.

virgile-dev commented 4 years ago

@ace thank you so much for your patience on this. Our back and forth are helping a lot. I agree that the milestones notifications are going to be a source of mis-understanding.

For now, I think that we should not modify how it works to keep it simple. We will adapt the locales so that it's less confusing for the user.

Moreover we are working on a spec for the AN that allows the admin to configure the milestone on the initiative type.

If we don't touch this, is there still much to do ?

ace commented 4 years ago

No problem. The only thing pending from the initial list is that UX issue on the admin when selecting the global scope. Then there's what you told me on our chat about the condition for the admin to be able to accept or reject an initiative depending on the votes. However, there are some things that I think are not working as expected. Let me try to comment them here.

How it is working now, given a parent and children scopes:

My issues comment above has this problems listed. A review of my findings would be great, just in case I misunderstood anything.

I already fixed a couple of them that resulted in errors and prevented me from even working on this. Should I go on fixing those issues or are there other priorities?

Mostly unrelated thing I just found while investigating how all this works, that feels strange to me. How votes are displayed on the initiative page. Only online votes are shown and counted for the progress bars. But for the 'need more votes / most popular initiative' text under the counter, the offline votes are also taken into account. So you can potentially see an initiative with not enough (online) votes and a not full progress bar, but 'most popular initiative'. Just to give some thought.

ace commented 4 years ago

One more thing, I just realized the changes in this PR break the sorting by signatures count we did in one of my first issues.

virgile-dev commented 4 years ago

@ace We gave it some thought with Alain :

First of all, we need to think separately of the signatures and the totals.

Example of setting for an initiative type

In the application the following scope structure is created :

The signatures are saved with the scope that is associated to the users. This is one thing.

Ex with structure above : signature saves "Berlaar".

About totals (counting the votes)

The totals (the parent and the child ones) are incremented every time a signature happens. Ex with structure above, user signs with "Berlaar", the parent initiative scopes have their totals incremented : Flanders + 1 / Global scope +1. This way the jauge have accurate totals. This is important performance wise. We save totals so that they are easily retrieved and don't have to be computed every time the page is displayed.

About edge cases

In this first version of the feature we are going to assume that the configuration has been done properly at the scope level /admin/scopes and at the initiative type scopes level. The config of the initiative should ensure that the totals are consistent : the sum of child scope totals should amount to the parent total.

To help the admins we can add help texts bellow the config flags associated with this feature and we will provide a read me with details and examples.

About offline votes

For now, this feature doesn't support offline votes.

About validation rules (totals) For now notifications about milestones should only be sent for parent level.

For status changes rejected and accepted :

The button to apply them in the admin should be visible if the goal (number of signatures required) are reached for the child scopes and the parent scope defined in the initiative type scopes

ace commented 4 years ago

Fixed the relevant issues and the calculations. https://github.com/decidim/decidim/pull/6143