PR#1950: Fixed Wrong data in heatmap issue #1949 - pagure

pagure

#1950 Fixed Wrong data in heatmap issue #1949

Closed 7 years ago by pingou. Opened 7 years ago by smitthakkar96.

smitthakkar96/pagure heatmap_fix into master

Changed log_action to record the datetime when action was performed.

smit thakkar • 7 years ago

e411771

Merge branch 'master' of ssh://pagure.io/pagure

smit thakkar • 7 years ago

72b1169

pagure/forms.py

file modified

		`@@ -99,6 +99,7 @@`


		`def ssh_key_validator(form, field):`
		`+ ''' Form for ssh key validation '''`
rahulbajaj commented 7 years ago you have made this change in each and every commit of yours
		`if not pagure.lib.are_valid_ssh_keys(field.data):`
		`raise wtforms.ValidationError('Invalid SSH keys')`

pagure/lib/__init__.py

file modified

+3 -4

		`@@ -3830,7 +3830,6 @@`

		`def log_action(session, action, obj, user_obj):`
		`''' Log an user action on a project/issue/PR. '''`
		`-`
rahulbajaj commented 7 years ago i dont think pep8 will accept this :)
		`project_id = None`
		`if obj.isa in ['issue', 'pull-request']:`
		`project_id = obj.project_id`
		`@@ -3845,9 +3844,9 @@`
		`user_id=user_obj.id,`
		`project_id=project_id,`
		`log_type=action,`
		`- ref_id=obj.id,`
		`- date=obj.date_created.date(),`
		`- date_created=obj.date_created`
		`+ ref_id=obj.id`
		`+ # date=obj.date_created.date(),`
		`+ # date_created=obj.date_created`
vibhcool commented 7 years ago you shall remove these lines instead of commenting them :)
pingou commented 7 years ago Why do you want to remove them in the first place?
pingou commented 7 years ago And my question highlights how you commit message needs to be adjusted so that someone understand what it is about without having to actually go and look at this ticket to understand the issue that is being resolved.
		`)`
		`if obj.isa == 'issue':`
		`setattr(log, 'issue_uid', obj.uid)`

smitthakkar96 commented 7 years ago

We were storing the date of the Issue/PR instead of the time when the issue was created, I modified log_action in init.py to store current datetime.

rahulbajaj commented on line 5 of pagure/lib/__init__.py 7 years ago

i dont think pep8 will accept this :)

rahulbajaj commented on line 5 of pagure/forms.py 7 years ago

you have made this change in each and every commit of yours

rahulbajaj commented 7 years ago

@smitthakkar96 can you please cf : https://pagure.io/pagure/pull-request/1936#comment-18995

vibhcool commented on line 18 of pagure/lib/__init__.py 7 years ago

you shall remove these lines instead of commenting them :)

pingou commented on line 18 of pagure/lib/__init__.py 7 years ago

Why do you want to remove them in the first place?

pingou commented on line 18 of pagure/lib/__init__.py 7 years ago

And my question highlights how you commit message needs to be adjusted so that someone understand what it is about without having to actually go and look at this ticket to understand the issue that is being resolved.

pingou commented 7 years ago

As mentioned inline, the commit message needs to be re-worded to be self explanatory.

In addition, it would be good to have unit-tests for this :)

smitthakkar96 commented 7 years ago

Ok thanks @pingou I will look into it and try to add tests.

farhaan commented 7 years ago

@smitthakkar96 what is the progress on this ?

smitthakkar96 commented 7 years ago

@farhaan I am travelling so I will need to work on it when I reach home.

smitthakkar96 commented 7 years ago

@pingou I will reword the commit message but I have removed those two lines as they were the causing the program to store the date of issue when it was created so I removed those lines, now by default the date and time of action will be stored as in models we have set default=datetime.datetime.utcnow() .

pingou commented 7 years ago

@smitthakkar96 I realized this when reading the ticket, and this shows why the commit message needs to be reworded :)

farhaan commented 7 years ago

@smitthakkar96 progress on this ?

pingou commented 7 years ago

ping?

pingou commented 7 years ago

@vivekanand1101 re-did this PR in https://pagure.io/pagure/pull-request/2072 so we could merge it :)

Thank you both!