Skip to content

Add INVALID status option#191

Merged
jsnshrmn merged 8 commits intoWikipediaLibrary:masterfrom
arcayn:master
Jan 4, 2019
Merged

Add INVALID status option#191
jsnshrmn merged 8 commits intoWikipediaLibrary:masterfrom
arcayn:master

Conversation

@arcayn
Copy link
Copy Markdown
Contributor

@arcayn arcayn commented Dec 9, 2018

This fixes T175249

Added ability for application status to be "Invalid", removed these applications from metrics/data analysis.

Copy link
Copy Markdown
Member

@Samwalton9 Samwalton9 left a comment

Choose a reason for hiding this comment

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

This is looking great, and from what I can see it works to the letter of the task.

The one hesitation I have, seeing how many places we're excluding Invalid applications throughout the site, is that it might be a more sensible direction to exclude invalid apps from Application querysets by default. You can see how this has been done to exclude unavailable resources by default at

class AvailablePartnerManager(models.Manager):
and

Since we rarely ever want to expose Invalid applications to users (except in one or two cases, see below), this might be a more practical approach. Could you give that a go? If you have any questions please let me know.

As for where we do want to show Invalid applications, I think it does actually make sense to show them on the Rejected tab of the Review page. They're effectively rejected, and this would allow coordinators to find them again more easily, without having to add a whole new tab for Invalid apps that I think would be unnecessary. This may require tweaking some tests such as

def test_list_rejected_object_visibility(self):
to include Invalid apps.

Users should also be able to see all their applications on their user page regardless if they're flagged Invalid, here:

context['object_list'] = editor.applications.all().order_by('status', '-date_closed')

@Samwalton9
Copy link
Copy Markdown
Member

Because Invalid applications are now listed on the Rejected page, we need to tag them as such (using the same bootstrap class as Rejected applications). Otherwise this looks good!

@arcayn
Copy link
Copy Markdown
Contributor Author

arcayn commented Dec 11, 2018

Just to clarify does that mean to use -danger for the INVALID entry on the LABELMAKER in applications/models.py?

Thanks :)

@Samwalton9
Copy link
Copy Markdown
Member

I believe so - then the functions in version_tags.py should handle the display.

@arcayn
Copy link
Copy Markdown
Contributor Author

arcayn commented Dec 11, 2018

Maybe we could use a different colour (e.g. the secondary grey I've included so far) so that users can easily tell the invalid and rejected apps on the same page apart? Just wondering

@Samwalton9
Copy link
Copy Markdown
Member

Sure - grey could work :)

@arcayn
Copy link
Copy Markdown
Contributor Author

arcayn commented Dec 12, 2018

Does my solution to use '-secondary' not work then? I believe that boostrap class should have a grey colour and can be used anywhere '-primary' can be

@Samwalton9
Copy link
Copy Markdown
Member

Ah, I missed that you'd already added -secondary. I had pulled your changes down and checked it and there was simply no tag for Invalid applications. From a little digging I can see that's because we're using Bootstrap 3.3, and secondary wasn't added as a colour option until version 4. If you just go with the same colour as rejected for now we can update this to secondary in the future when we've updated to 4 :)

@arcayn
Copy link
Copy Markdown
Contributor Author

arcayn commented Dec 12, 2018

Ah I had no idea! Will change

@Samwalton9
Copy link
Copy Markdown
Member

This looks great! I wouldn't mind getting a 2nd look over by @jsnshrmn when he's back in January in case this has consequences I haven't considered.

If you're interested in continuing to contribute to the platform you can find further open tasks at https://phabricator.wikimedia.org/project/view/2765/ :)

@Samwalton9 Samwalton9 requested a review from jsnshrmn January 3, 2019 18:53
Copy link
Copy Markdown
Member

@jsnshrmn jsnshrmn left a comment

Choose a reason for hiding this comment

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

This looks great, and it does what it says on the tin. Apologies for the length of time it sat unmerged.
I'm dropping the auto migrations out of this, as our continuous integration environment now adds those in as needed while preventing the possibility of multiple leaf nodes in the migration graph (migration conflicts). Thanks so much for your contribution!

@jsnshrmn jsnshrmn merged commit e73f303 into WikipediaLibrary:master Jan 4, 2019
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