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

Ks 2022 09 support phase #4545

Merged
merged 5 commits into from
Oct 17, 2022
Merged

Ks 2022 09 support phase #4545

merged 5 commits into from
Oct 17, 2022

Conversation

Rineee
Copy link
Contributor

@Rineee Rineee commented Oct 5, 2022

@fuzzylogic2000 Maybe you already want to have a look, especially the rules (left it all in mb for now), but there a still some todos:

  • remove ratings from list view (-> add supoort there?)
  • remove ratings from map pop up (-> add support there?)
  • change filter wording for support phase

Not even sure about the 'adding support' part of the first two here, is that included in the story? Not explicitly at least :)

@Rineee Rineee changed the title Ks 2022 09 support phase WIP Ks 2022 09 support phase Oct 5, 2022
Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

Looks good! Even though there is still the stuff we need to think about and handle, it looks like a good way to me

Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

@Rineee I thought handling as much as possible in the python code is a good idea? Then it's also easy to change when the logic when we show what is changed. (like in another phase as well or between certain ones or whatever.) What do you think?

commentCount={proposal.comment_count}
/>
)}
<ListItemStats
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove is_voting_phase from the template tag as we have only used it here. But it is still in the tests, so they will definitely break. And I cannot remember how to fix them. Can you show me again @khamui or @goapunk ?

@Rineee Rineee force-pushed the ks-2022-09-support-phase branch from 4bf9709 to 9dfd077 Compare October 17, 2022 09:07
@Rineee
Copy link
Contributor Author

Rineee commented Oct 17, 2022

@Rineee I thought handling as much as possible in the python code is a good idea? Then it's also easy to change when the logic when we show what is changed. (like in another phase as well or between certain ones or whatever.) What do you thin

@fuzzylogic2000 Really cool, I like it this way. I also tried to fix the frontend tests, hope the right way 🙈 I would say we could merge now and do the pop up part once it is fixed?

@Rineee Rineee changed the title WIP Ks 2022 09 support phase Ks 2022 09 support phase Oct 17, 2022
@fuzzylogic2000
Copy link
Contributor

@Rineee I thought handling as much as possible in the python code is a good idea? Then it's also easy to change when the logic when we show what is changed. (like in another phase as well or between certain ones or whatever.) What do you thin

@fuzzylogic2000 Really cool, I like it this way. I also tried to fix the frontend tests, hope the right way see_no_evil I would say we could merge now and do the pop up part once it is fixed?

@Rineee Yes, I think merging now and fixing the map popups in the second map is a good idea! But I think one of @phillimorland @khamui and @goapunk should also approve and do the merging. :)

@philli-m philli-m self-assigned this Oct 17, 2022
test('displaying all 3 stats', () => {
test('rating displaying all 3 stats', () => {
const permissions = {
view_support_count: false,
Copy link
Contributor

@philli-m philli-m Oct 17, 2022

Choose a reason for hiding this comment

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

should this be true to show all 3 stats or have I misunderstood the name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, as we never show all stats on the real list, I don't know. Maybe there should be two tests, one showing the supports and the other one the rating?

Copy link
Contributor

Choose a reason for hiding this comment

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

then we can change the description of this test to rating phase showing 2 stats

Copy link
Contributor

Choose a reason for hiding this comment

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

oh no, overread that. Yes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phillimorland @fuzzylogic2000 So this test is testing if its showing all 3 stats (pos, neg, comments) when there is a rating phase and the other one below is testing that only 2 are shown (pos, comments) when there is support.. does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it is true to show all 3, see lines 20-25.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, cool. Just delete the task again. :)

Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

@fuzzylogic2000 @Rineee code looks good and tests pass, just one quick question about the 3 stats. Also should the default phase descriptions in dashboard be updated? and also how do I give myself support permission? couldn't figure that out in dashboard

@philli-m philli-m removed their assignment Oct 17, 2022
@fuzzylogic2000
Copy link
Contributor

@fuzzylogic2000 @Rineee code looks good and tests pass, just one quick question about the 3 stats. Also should the default phase descriptions in dashboard be updated? and also how do I give myself support permission? couldn't figure that out in dashboard

@phillimorland Oh, sorry, should have said! The supporting only works with a new 3 phase Bürgerhaushalt as the old one still has the old phase.

@philli-m
Copy link
Contributor

@fuzzylogic2000 @Rineee code looks good and tests pass, just one quick question about the 3 stats. Also should the default phase descriptions in dashboard be updated? and also how do I give myself support permission? couldn't figure that out in dashboard

@phillimorland Oh, sorry, should have said! The supporting only works with a new 3 phase Bürgerhaushalt as the old one still has the old phase.

@fuzzylogic2000 cheers, looks good, only thing is the support and the moderation buttons are on 2 separate lines but that can be fixed later so LGTM from me :)

@fuzzylogic2000
Copy link
Contributor

I will add a task about the button and merge.

@fuzzylogic2000 fuzzylogic2000 merged commit d2ccd36 into main Oct 17, 2022
@fuzzylogic2000 fuzzylogic2000 deleted the ks-2022-09-support-phase branch October 17, 2022 15:13
@Rineee
Copy link
Contributor Author

Rineee commented Oct 18, 2022

@fuzzylogic2000 @Rineee code looks good and tests pass, just one quick question about the 3 stats. Also should the default phase descriptions in dashboard be updated? and also how do I give myself support permission? couldn't figure that out in dashboard

@phillimorland Oh, sorry, should have said! The supporting only works with a new 3 phase Bürgerhaushalt as the old one still has the old phase.

@fuzzylogic2000 cheers, looks good, only thing is the support and the moderation buttons are on 2 separate lines but that can be fixed later so LGTM from me :)

Oh yes, I saw that and then ignored it 🙈 Good you pointed it out again @phillimorland

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.

3 participants