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

Show recently filed applications and their date #529

Merged
merged 1 commit into from
Oct 5, 2020
Merged

Show recently filed applications and their date #529

merged 1 commit into from
Oct 5, 2020

Conversation

lalit97
Copy link
Contributor

@lalit97 lalit97 commented Sep 18, 2020

Description

Show all applications filed in last 3 months and their submission date, on application evaluation page.

Rationale

Before this we used to show 5 recent applications on evaluation page. But we need to display more applications on that page so coordinators can decide if a user is applying indiscriminately.

Phabricator Ticket

T262904

How Has This Been Tested?

I have added 2 new tests for this behaviour. Also removed a test which was previously used to test rendering of 5 recent applications only.
This change can be tested manually by going on application evaluation page and checking recent applications section.

Screenshots of your changes:

Screen Shot 2020-09-18 at 6 31 53 PM

Types of changes

What types of changes does your code introduce? Add an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@lalit97
Copy link
Contributor Author

lalit97 commented Sep 18, 2020

I have broke down the task of showing recent applications in following 2 ways

(1) If there are more than 5 applications present in the period of last 3 months than I thought it will be good if we display all of them instead of just limiting to 5 applications. Because that will give coordinators a more clear picture of if an editor has applied indiscriminately.

(2) If there are less than 5 applications present in the period of last 3 months than I thought it will be good if we add some more applications from past i.e. applications which are older than 3 months time period. So that we can show at least 5 recent applications.
But if there were less than 5 total applications made by an editor than we have to show less than 5 recent applications.

Also the diff of applications/tests.py is not showing properly I have tried to make separate commits and to make the diff look better by deleting the previous test and then adding the 2 new tests. But they are mixing somehow.

@Samwalton9
Copy link
Member

If there are more than 5 applications present in the period of last 3 months than I thought it will be good if we display all of them instead of just limiting to 5 applications. Because that will give coordinators a more clear picture of if an editor has applied indiscriminately.

I think this makes sense to me, good suggestion.

If there are less than 5 applications present in the period of last 3 months than I thought it will be good if we add some more applications from past i.e. applications which are older than 3 months time period. So that we can show at least 5 recent applications.

I think this, however, over-complicates things. We should show coordinators as little information as necessary to make a decision - seeing applications made more than 3 months ago doesn't, in my opinion, help them make a decision. So I think it should only show last-3-months applications in this case.

Also the diff of applications/tests.py is not showing properly I have tried to make separate commits and to make the diff look better by deleting the previous test and then adding the 2 new tests. But they are mixing somehow.

One of the engineers can confirm, but I think this is just a quirk of how Github is calculating the diff, I wouldn't worry :)

@lalit97
Copy link
Contributor Author

lalit97 commented Sep 22, 2020

I think this makes sense to me, good suggestion.

Thanks :)

I think this, however, over-complicates things. We should show coordinators as little information as necessary to make a decision - seeing applications made more than 3 months ago doesn't, in my opinion, help them make a decision. So I think it should only show last-3-months applications in this case.

Okay got your point will make changes according to this. Also I have a design query if for an editor there are 0 applications found in last-3-months then should we show an empty table in Recent Applications sections or should we do not show the whole Recent Applications section(i.e. not even the heading) or maybe we can show a message below the heading that no applications are made recently. Which one should I pick for this case?

One of the engineers can confirm, but I think this is just a quirk of how Github is calculating the diff, I wouldn't worry :)

Okay if you or any other member does not face any difficulty while reviewing the changes then it is okay, otherwise I will try to come up with some solution if there is any :)

@Samwalton9
Copy link
Member

Also I have a design query if for an editor there are 0 applications found in last-3-months then should we show an empty table in Recent Applications sections or should we do not show the whole Recent Applications section(i.e. not even the heading) or maybe we can show a message below the heading that no applications are made recently. Which one should I pick for this case?

In this case I think it would be best to show no table or information.

Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! This is looking great, but some changes need to be made in order to merge this 🙂

Show only recently filed applications, and their submission date, on application evaluation pages

Bug: T262904
Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

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

Thank you for the changes you made! This LGTM!

@suecarmol suecarmol merged commit e139a9f into WikipediaLibrary:master Oct 5, 2020
@lalit97
Copy link
Contributor Author

lalit97 commented Oct 6, 2020

Thank you :)

@lalit97 lalit97 deleted the T262904 branch October 6, 2020 05:28
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