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

[bug 1078233] Change Rep of the Month date #1444

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Mte90
Contributor

Mte90 commented Mar 14, 2018

screenshot-2018-3-14 mozilla reps - add new featured rep

I am not sure about how print a date picker but this feature is used only by administrators so I don't think that is so important.

Mte90 added some commits Mar 14, 2018

@MichaelKohler

This comment has been minimized.

Collaborator

MichaelKohler commented Mar 15, 2018

I'll have a look at this tonight.

@@ -33,7 +33,7 @@
</span>
{{ field_errors(form.users) }}
</div>
<div class="large-12 columns">
<div class="large-6 columns">

This comment has been minimized.

@MichaelKohler

MichaelKohler Mar 15, 2018

Collaborator

can we leave this at 12? Then it's easier to proof-read the text. I guess the date can be moved up so it shares the space with the reps list (so, reps list = 6, date field = 6, textarea = 12). What do you think?

This comment has been minimized.

@Mte90

Mte90 Mar 16, 2018

Contributor

The screen that I made is for the new interface.
This was before so I was trying to improve the ui
screenshot_20180316_121806

</div>
<div class="large-6 columns">
<input type="text" class="flat" value="{{ form.created_on.value() }}"
name="created_on" />

This comment has been minimized.

@MichaelKohler

MichaelKohler Mar 15, 2018

Collaborator

can you align the name attribute directly under the "type" attribute?
also, can you add a placeholder for the input field that mentions the format of the date that is expected? I think it's okay to not have a datepicker here, but I think we should show the expected format. This also means that we need to filter out the value if there is none set (otherwise it will overwrite the placeholder with "None").

@MichaelKohler

This comment has been minimized.

Collaborator

MichaelKohler commented Mar 15, 2018

Thanks, there are a few code things to improve and I also found an error. I've created an entry for March (2018-03-01) and then went back and changed it to 2018-04-15. While it got saved correctly, the entry on the Reps profile still shows "Featured Rep for March 2018" and doesn't reflect the new date.

screen shot 2018-03-15 at 23 41 22

screen shot 2018-03-15 at 23 41 29

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Mar 16, 2018

Seems that add a field to change the date is not enough, I will investigate better, the issue that locally to get a rep of the month environment is not easy, is checking a lot of stuff so I was commenting code to do that.

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Mar 16, 2018

As I can see on my system is working, changing the date on future or past of a different month changed it without issue.

@@ -33,6 +33,11 @@
</span>
{{ field_errors(form.users) }}
</div>
<div class="large-6 columns">
<input type="text" class="flat" value="{{ form.created_on.value() }}"
name="created_on" placeholder="YYYY-MM-DD HH:MM:SS" />

This comment has been minimized.

@MichaelKohler

MichaelKohler Mar 17, 2018

Collaborator

Thanks for the fixes. The layout looks better now. This placeholder here doesn't work though, as it shows "None" for me. I think that's the return value of form.created_on.value().

@MichaelKohler

This comment has been minimized.

Collaborator

MichaelKohler commented Mar 17, 2018

As I can see on my system is working, changing the date on future or past of a different month changed it without issue.

  1. Add new RoM

screen shot 2018-03-17 at 14 28 11

  1. Save and check profile -> all good

screen shot 2018-03-17 at 14 28 27

  1. Edit RoM again and change date to past

screen shot 2018-03-17 at 14 28 45

  1. Still shows "March"

screen shot 2018-03-17 at 14 29 00

This happens every time I edit it. I think if we allow it to have a date, we also should allow edits on it.

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Mar 17, 2018

This is my recording that show that is working on my local system.
https://youtu.be/Tc6pmtKlWF0

I saw the issue with None showed on new post. I think that we can hide the field on new submission and show only on editing is the best way also because we not publish rotm for different months all together.

@MichaelKohler

You are right, seems to have been the cache. Sorry about that. Also the placeholder works now, so this is ready for a merge. Thanks a lot!

@akatsoulas this is ready for your final review and merge, thanks!

@@ -33,6 +33,11 @@
</span>
{{ field_errors(form.users) }}
</div>
<div class="large-6 columns">
<input type="text" class="flat" value="{{ form.created_on.value()|default("", True) }}"
name="created_on" placeholder="YYYY-MM-DD HH:MM:SS" />

This comment has been minimized.

@akatsoulas

akatsoulas Mar 20, 2018

Contributor

Instead of using input here you can use the field_with_attrs(form.created_on).

This comment has been minimized.

@Mte90

Mte90 Mar 20, 2018

Contributor

I didn't know that, I will do asap

This comment has been minimized.

@akatsoulas

This comment has been minimized.

@Mte90

Mte90 Mar 21, 2018

Contributor

I made that change

@@ -33,6 +33,10 @@
</span>
{{ field_errors(form.users) }}
</div>
<div class="large-6 columns">
{{ field_with_attrs(form.created_on, placeholder='YYYY-MM-DD HH:MM:SS', value=form.created_on.value()|default("", True)) }}
<div class="required-field"></div>

This comment has been minimized.

@akatsoulas

akatsoulas Mar 27, 2018

Contributor

Please also add {{ field_errors(form.created_on) }} here.

This comment has been minimized.

@Mte90

Mte90 Mar 28, 2018

Contributor

added

@akatsoulas

This comment has been minimized.

Contributor

akatsoulas commented Mar 27, 2018

You will also need to edit the models.py file [0] to add some logic in the save() method otherwise you won't be able to save the new date because it will be always overridden with now().

You could add something like:

if not self.pk and not self.created_on`

in this line [1].

[0] https://github.com/mozilla/remo/blob/master/remo/featuredrep/models.py
[1] https://github.com/mozilla/remo/blob/master/remo/featuredrep/models.py#L33

@akatsoulas

This comment has been minimized.

Contributor

akatsoulas commented Mar 27, 2018

The value passed by the created_on field is not validated via a clean method. This allowed me to enter an invalid date (eg 2038-1-1). Ideally we need to add a clean method to validate at least that the date specified is not in the future.

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Mar 28, 2018

I tried right now and I cannot put a wrong date but I can put on the future.
The idea is that the council can schedule the reps of the month of the next month so we need to support the change to another date in the future.

@akatsoulas

This comment has been minimized.

Contributor

akatsoulas commented Mar 29, 2018

I believe that the best approach here is to clean the date that the user inputs. We can allow dates that will be only one month in the future and reject everything else that it's beyond that. For example I was able to add the following date which is wrong.

screenshot-2018-3-29 mozilla reps - featured reps

@akatsoulas

This comment has been minimized.

Contributor

akatsoulas commented Mar 29, 2018

Moreover, we need extra logic for the dashboard in order to not display featured reps that have a date in the future.

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Mar 29, 2018

A limit of a month seems reasonable, I will work on that.

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Mar 29, 2018

This commit block update if the date written is 30 days in the future.
I think that we have to display the featured reps configured for the future to help on planning.

if not self.updated_on:
self.updated_on = now()
if not self.pk:
self.created_on = now()
super(FeaturedRep, self).save(*args, **kwargs)
if self.created_on < now() + datetime.timedelta(days=30):

This comment has been minimized.

@akatsoulas

akatsoulas Mar 30, 2018

Contributor

This will just block the update without notifying the users why this happened. What we need to do is to validate this field in the clean method in the form and inform users that they cannot use dates more than a month in the future than the current date.

This comment has been minimized.

@Mte90

Mte90 Mar 30, 2018

Contributor

You are right, there is a way to report issues on the backend?
In the clean method is the right way?

This comment has been minimized.

@akatsoulas

akatsoulas Mar 30, 2018

Contributor

Correct, you can add either a def clean method in the form [0] or you can clean only this specific field by adding a def clean_created_on method in the same form.

[0] https://github.com/mozilla/remo/blob/master/remo/featuredrep/forms.py#L9

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Mar 30, 2018

Done, now there is an alert inside the editing about the date in the future.

@MichaelKohler

Thanks for your efforts here, Daniele. I've made a few more comments. Will test once those are addressed.

@@ -732,8 +732,8 @@ label.required:after {
text-align: left;
}
small.error {
width: 50%;
padding: 0;
width: 100%;

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 1, 2018

Collaborator

can you please test this change with other errors that we show? For example on the profile page or a new event page?

This comment has been minimized.

@Mte90

Mte90 Apr 1, 2018

Contributor

That rule involve only the featured-reps page and not the others and now with the new layout we need to change it.

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 1, 2018

Collaborator

oh, yes, sorry!

@@ -13,7 +16,15 @@ class FeaturedRepForm(happyforms.ModelForm):
userprofile__registration_complete=True,
groups__name='Rep').order_by('first_name'))
def clean_created_on(self):
print(self.cleaned_data['created_on'])

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 1, 2018

Collaborator

can you remove those two prints?

This comment has been minimized.

@Mte90

Mte90 Apr 1, 2018

Contributor

doh

print(self.cleaned_data['created_on'])
print(now() + datetime.timedelta(days=30))
if self.cleaned_data['created_on'] > now() + datetime.timedelta(days=30):
raise ValidationError('The date cannot be over 30 days in the future.')

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 1, 2018

Collaborator

minor: "cannot be more than 30 days in the future". I'm not sure if 30 is the right value here, is there a way to do a "proper" month?

This comment has been minimized.

@Mte90

Mte90 Apr 1, 2018

Contributor

actually no because we are doing a sum of 30 to the date of today.

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 1, 2018

Collaborator

Which does not work out for months with 30 days as well as February, correct? ;)

This comment has been minimized.

@Mte90

Mte90 Apr 1, 2018

Contributor

right but I think that we need that feature now and later we can improve and find a better way.
In php is different the management of dates so you don't need to do sums or put days for this kind of stuff. In few words I don't know if there is a better way.

@@ -2,7 +2,6 @@
from django.db import models
from django.utils.timezone import now

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 1, 2018

Collaborator

this leads to a failing flake8 lint: https://travis-ci.org/mozilla/remo/jobs/360278965

@MichaelKohler

As tasos mentioned [here)(https://github.com//pull/1444#issuecomment-376518264) currently every "save" leads to be saved as now().

Additionally, as tasos mentioned here we will need to make sure that we don't show upcoming RoM entries on the front page.

Can you fix those 2 issues?

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Apr 9, 2018

For the first point I can do it, I had implementd but with the new code I forgot it to readd it.

For the second point what is the best procedure? If I configure a ROM for the next month the council/peers needto see that but not the rest of reps. I am right?

@MichaelKohler

This comment has been minimized.

Collaborator

MichaelKohler commented Apr 9, 2018

For the second point what is the best procedure? If I configure a ROM for the next month the council/peers needto see that but not the rest of reps. I am right?

For the front page I think it should never show up if the date is in the future. For the list, entries in the future should only be shown if the user has the "can_edit_featured" permission, the same as for the "Add new" button as seen here: https://github.com/mozilla/remo/blob/master/remo/featuredrep/templates/featuredrep_list.jinja#L26

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Apr 9, 2018

The new code add the filtering on the view for people with permissions and fix the issue of now().

@MichaelKohler

I've added a few comments. Additionally here are some test cases:

  • Log in as Council member, add new RoM for today
  • Create a new RoM for next month
  • Go to RoM overview and check that you can see both entries
  • Go to the frontpage and check that you only see the one from today, not the one from next month
  • Leave the form empty -> should display errors nicely
  • Add date > 30 days in the future -> should shout at you in a nice red error box
  • Log in as normal Rep, check that you can only see the one from today in the overview
  • Check that you can only see the one from today on the frontpage
@cache_control(private=True, max_age=60 * 10)
def list_featured(request):
"""List all Featured Reps."""
featured_list = FeaturedRep.objects.all()
if not request.user.has_perms('featuredrep.can_edit_featured'):

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 14, 2018

Collaborator

this always is true as has_perms (at least for me here) always returns false. This is for a user in the "Council" group. I can't tell you right now why though.. But this leads to the following problem:

screen shot 2018-04-14 at 17 46 14

As you can see, I see the button on the top right, but I don't see all existing entries:

screen shot 2018-04-14 at 17 13 37

In any case, why should people who do not have permission to edit the featured reps only see the ones in the future? Shouldn't that be only the ones from today and in the past?

IMHO, with permissions you should see all, without permission you only see today+past

This comment has been minimized.

@Mte90

Mte90 Apr 14, 2018

Contributor

The idea was to not show in the list to the people not council the scheduled featured rep.

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 14, 2018

Collaborator

correct, but this means that normal Reps should see everything from today to the past. As it is now, they will see all in the future (and just those). This is 100% the opposite of what we want ;)

This comment has been minimized.

@Mte90

Mte90 Apr 14, 2018

Contributor

ops I have to revert but seems easy to fix

This comment has been minimized.

@Mte90

Mte90 Apr 14, 2018

Contributor

Can you check the latest commit for this point?

This comment has been minimized.

@Mte90

Mte90 Apr 23, 2018

Contributor

As I can see the non-council cannot see the future reps and as council I can see them.
I have an account that is only rep and I see only one.

This comment has been minimized.

@MichaelKohler

MichaelKohler May 8, 2018

Collaborator

I will test this again.

@@ -13,7 +16,13 @@ class FeaturedRepForm(happyforms.ModelForm):
userprofile__registration_complete=True,
groups__name='Rep').order_by('first_name'))
def clean_created_on(self):
if self.cleaned_data['created_on'] > now() + datetime.timedelta(days=30):

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 14, 2018

Collaborator

this fails if the user didn't add any date (leave the whole form empty to test):

screen shot 2018-04-14 at 17 09 04

This comment has been minimized.

@Mte90

Mte90 Apr 14, 2018

Contributor

Cool, so I have to add a check if isset.

featured_list = FeaturedRep.objects.all()
if not request.user.has_perms('featuredrep.can_edit_featured'):
featured_list = FeaturedRep.objects.filter(created_on__gte=datetime.today())

This comment has been minimized.

@MichaelKohler

MichaelKohler Apr 14, 2018

Collaborator

adding the check here is not enough, that only works for the list itself (see my comment above for that part). However, users would still see future RoM on the frontpage:

screen shot 2018-04-14 at 17 49 29

This comment has been minimized.

@Mte90

Mte90 Apr 14, 2018

Contributor

argh I missed that part!

This comment has been minimized.

@Mte90

Mte90 Apr 23, 2018

Contributor

I can confirm that now is fixed show only the right profiles

Mte90 added some commits Apr 14, 2018

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Apr 14, 2018

Ok now we have to understand why the tests are failing because I have no idea XD

featured_list = FeaturedRep.objects.all()
if not request.user.has_perms('featuredrep.can_edit_featured'):
featured_list = FeaturedRep.objects.filter(created_on__lte=datetime.today())

This comment has been minimized.

@Mte90

Mte90 Apr 23, 2018

Contributor

This seems the origin of errors on test, with this the unit tests are failing maybe because this data is not available.

This comment has been minimized.

@Mte90

Mte90 Apr 30, 2018

Contributor

@MichaelKohler do you have any suggestions?

This comment has been minimized.

@MichaelKohler

MichaelKohler May 8, 2018

Collaborator

As far as I see, the error comes from

featured_rep = FeaturedRep.objects.filter(created_on__lte=datetime.today()).latest()

in remo/base/views.py and not from here.

I'd guess that the fixtures don't include an entry and therefore .latest() can't return anything and therefore there is no matching query. Adding an entry to the test fixtures should solve this (haven't tried though).

This comment has been minimized.

@Mte90

Mte90 May 8, 2018

Contributor

https://github.com/mozilla/remo/blob/master/remo/featuredrep/fixtures/demo_featured.json contain an entry and lte is <= so have to work.
I am not an expert of fixtures but we cannot have dinamyc fixtures to have the date of today so we can tests also this cases?

@MichaelKohler

I'll need to test this again once you have fixed everything. There are two things I'd like you to fix, see above.

Thanks!

@@ -13,7 +16,17 @@ class FeaturedRepForm(happyforms.ModelForm):
userprofile__registration_complete=True,
groups__name='Rep').order_by('first_name'))
def clean_created_on(self):
if 'created_on' in self.cleaned_data:
nowplusthirthy = now() + datetime.timedelta(days=30)

This comment has been minimized.

@MichaelKohler

MichaelKohler May 8, 2018

Collaborator

I think snake case would fit the general pattern better: now_plus_thirty_days

@cache_control(private=True, max_age=60 * 10)
def list_featured(request):
"""List all Featured Reps."""
featured_list = FeaturedRep.objects.all()
if not request.user.has_perms('featuredrep.can_edit_featured'):

This comment has been minimized.

@MichaelKohler

MichaelKohler May 8, 2018

Collaborator

I will test this again.

featured_list = FeaturedRep.objects.all()
if not request.user.has_perms('featuredrep.can_edit_featured'):
featured_list = FeaturedRep.objects.filter(created_on__lte=datetime.today())

This comment has been minimized.

@MichaelKohler

MichaelKohler May 8, 2018

Collaborator

As far as I see, the error comes from

featured_rep = FeaturedRep.objects.filter(created_on__lte=datetime.today()).latest()

in remo/base/views.py and not from here.

I'd guess that the fixtures don't include an entry and therefore .latest() can't return anything and therefore there is no matching query. Adding an entry to the test fixtures should solve this (haven't tried though).

@MichaelKohler

This comment has been minimized.

Collaborator

MichaelKohler commented Jun 10, 2018

@Mte90 what's the status on this one here?

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Jun 12, 2018

There was problems with unit tests and fixtures and I am not so expert about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment