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

Filter by my own repositories at Import Remote Project #3548

Merged
merged 16 commits into from Mar 9, 2018

Conversation

Projects
None yet
4 participants
@humitos
Member

humitos commented Jan 25, 2018

Missing things:

  • upload the final minified file by running gulp build
  • define HTML layout for the "Filter by my own User" link
  • make Next and Previous work
  • create the proper css with less for the filters (we are using the remote-org)
  • show the social network (github, bitbucket, gitlab) next to the account
  • do we want to show the social account for all the organizations?

Closes #3337

own = self.request.query_params.get('own', None)
if own is not None:
query = query.filter(organization__isnull=True)

This comment has been minimized.

@safwanrahman

safwanrahman Jan 25, 2018

Member

I think it can be written something like
query = query.filter(organization=None)

@humitos

Another issue that I found is that when you hit https://readthedocs.org/dashboard/import/ this endpoint is called twice:

https://readthedocs.org/api/v2/remote/repo/

So, I think the depencies are not polished enough.

self.urls['remoterepository-list'],
{org: org}
);
if (!self.page_current()) {

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

While working on this PR I found another issue:

  1. go https://readthedocs.org/dashboard/import/
  2. select an org to filter by
  3. click next

you will note that the URL that is requested by AJAX never changes

I added this if here to fix that.

self.set_filter_own = function () {
// TODO: Since we are modifying three observable items this
// trigger the request three times (because of the computed
// value). How I can fix it?

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

I these two functions I have the same problem. I'm modifying these observable objects that are used by the computed value, so I'd like to know if there is a way to modify all of them in an atomic way and launch just one computation.

class="remote-org"
data-bind="click: function () { $root.set_filter_own(); }">
<!-- TODO: show all the connected accounts here with corresponding avatar -->
<span>humitos (GitHub)</span>

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

Here, I want to list all of the social account connected and list them here.

I was thinking on creating a new JS object for this (like Organization) and do a query at init to populate the list. Sounds like a good idea?

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

I solved this by using Django allauth templates to generate each link.

@humitos

This comment has been minimized.

Member

humitos commented Jan 25, 2018

Another issue that I found is that when you hit readthedocs.org/dashboard/import this endpoint is called twice

Fixed in the latest commit.

@humitos

This comment has been minimized.

Member

humitos commented Jan 25, 2018

Example,

captura de pantalla_2018-01-25_18-54-42

@@ -157,14 +157,23 @@ function ProjectImportView (instance, config) {
ko.computed(function () {
var org = self.filter_org(),
orgs = self.organizations(),

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

I removed this dependency because it was useless and produce hitting the API twice at init.

{% for type, list in accounts.items %}
{% for account in list %}
<li
class="remote-org"

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

Use a proper CSS class for this like social-account maybe?

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

I don't think social-account is what we want, but remote-user could be added. A css class social-account refers to the account connected to GitHub/Bitbucket

This comment has been minimized.

@humitos

humitos Jan 30, 2018

Member

I will need help here, since the remote-user doesn't exist and the remote-org is defined in the import.less file. I'd say that they should be almost the same. So, I will need some guideance to not repeat the whole thing.

<img src="{{ account.get_avatar_url }}" class="remote-org-avatar" />
<!-- TODO: get the proper username for the connected account -->
<span>{{ account.user.username }} ({{ account.get_provider_display }})</span>

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

I wasn't able to find the username not in our database, but the social account one. How I can get it?

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

This isn't on the remote repository object return, in the .json attribute? You might need to write a general property to fetch this based on the provider

This comment has been minimized.

@humitos

humitos Jan 26, 2018

Member

Yes, the extra_data has this information but I didn't find a generic way to access it (which I suppose that there should be) since bitbucket is username and github is login and I don't know which one is for gitlab.

I think it's kind of weird that this information is not being accessible in a generic way. get_avatar_url exists and does the trick for the avatar; that's why I thought this should exist.

{% get_social_accounts user as accounts %}
{% if accounts %}
<h3>{% trans "Filter by Own Repositories" %}</h3>
{% for type, list in accounts.items %}

This comment has been minimized.

@humitos

humitos Jan 25, 2018

Member

By populating this by Django templates, we loose the ability to use observers and enable/disable the CSS class that make it gray. What should we do here?

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

This should be driven by knockout in the same way that the organization listing is driven by knockout.

The best way to handle this might be to make the array of repositories an array of objects that specify if the object is a user repository or an organization repository. The organization filtering would be repurposed to know about both.

User repositories should come first in the array, so that listing is consistent.

This comment has been minimized.

@humitos

humitos Jan 26, 2018

Member

This makes sense to me. I didn't know how we want to implement this

  • a new endpoint to retrieve all the user accounts?
  • a <script> tag defining the JS object with django template tags?
  • any other idea

This comment has been minimized.

@humitos

humitos Jan 30, 2018

Member

I created a new endpoint at a9d98a2

@agjohnson

This seems like a great start! I think the interface needs a complete overhaul, so I'd like to address with a focus on a more searchable interface. I think this is a great intermediate step though.

I would rather that the repositories are all handled with knockout and that we are avoiding django templating for this. If you need help with it, let me know. I'm happy to provide a bit more direction here if you need it.

{# accounts is a dictionary with the provider as key and a list of connected accounts of that provider #}
{% get_social_accounts user as accounts %}
{% if accounts %}
<h3>{% trans "Filter by Own Repositories" %}</h3>

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

There are two options here that I'd prefer over a heading like this:

  • Make this heading "My repositories"
  • Remove the second heading and just have user and organization filtering under a heading "Filter repositories"

I lean towards the second, it's probably not necessary to be that explicit with headings here.

This comment has been minimized.

@davidfischer

davidfischer Jan 26, 2018

Contributor

If you choose to combine, make sure to specify that the organization comes from a specific social account. For example, "Read the Docs (github)" or the like.

This comment has been minimized.

@humitos

humitos Feb 1, 2018

Member

I'm not sure about the idea of combining them from a UX perspective. I think it's too much clear for the user if we have two sections. Maybe "My repositories" and "My organizations"?

Also I started trying to make just one list and I think it complicates the code a little more.

Besdies, by adding the (social account) to each item in the list will be too verbose I think. Don't you think? I'd say that it will be rare to have a two organizations with the same name under different services. Or maybe, we can add the name of the social account only if this happens? (that will be more complicated to code).

Just thinking aloud. I don't have an strong opinion yet and I will probably change my mind while I write the code since knockout is new to me :)

{% for type, list in accounts.items %}
{% for account in list %}
<li
class="remote-org"

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

I don't think social-account is what we want, but remote-user could be added. A css class social-account refers to the account connected to GitHub/Bitbucket

{% for account in list %}
<li
class="remote-org"
data-bind="click: function () { $root.set_filter_own('{{ account.provider }}'); }">

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

Any reason this needs to be $root? filter_own should exist on the object context used in this view.

Edit: I see now what happened. It looks like this is the same code as below, but this code doesn't exist in a knockout driven iterator. You only need something like this to reference the root or parent object inside an iterator. We'll need this pattern when we replace the django template driven code above though.

<img src="{{ account.get_avatar_url }}" class="remote-org-avatar" />
<!-- TODO: get the proper username for the connected account -->
<span>{{ account.user.username }} ({{ account.get_provider_display }})</span>

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

This isn't on the remote repository object return, in the .json attribute? You might need to write a general property to fetch this based on the provider

@@ -188,7 +197,7 @@ function ProjectImportView (instance, config) {
.always(function () {
self.is_ready(true);
});
});
}).extend({ deferred: true });

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

I had to look up usage of deferred here. This looks like the behavior we want around AJAX requests.

This comment has been minimized.

@humitos

humitos Jan 26, 2018

Member

Yes. In the documentation there is exactly the same example that our case: http://knockoutjs.com/documentation/deferred-updates.html

self.filter_org(id);
self.filter_own(false);

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

I think we can get around the need for deferred updates here if we avoid resetting both the filter_org and filter_own. i think this means we instead have a singular filter_by() observable. This would remove the duplicated updates to the observable performing the ajax request.

This comment has been minimized.

@humitos

humitos Feb 1, 2018

Member

I'm not too convinced of having just one list for the filters yet.

We can't remove the deferred updates because we need to modify the page_current also.

// This needs to use deferred updates
self.filter_own(account);
self.filter_org(null);

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

Same as above here.

{% get_social_accounts user as accounts %}
{% if accounts %}
<h3>{% trans "Filter by Own Repositories" %}</h3>
{% for type, list in accounts.items %}

This comment has been minimized.

@agjohnson

agjohnson Jan 26, 2018

Contributor

This should be driven by knockout in the same way that the organization listing is driven by knockout.

The best way to handle this might be to make the array of repositories an array of objects that specify if the object is a user repository or an organization repository. The organization filtering would be repurposed to know about both.

User repositories should come first in the array, so that listing is consistent.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jan 26, 2018

Overall I was able to filter repositories. However, I wouldn't mind a bit more UI feedback. For example, there's no feedback that a given filter is active. This is especially confusing because clicking a filter a second time toggles it off.

screen shot 2018-01-26 at 11 04 00 am

I also wouldn't mind feedback that the organization comes from github.

@humitos

This comment has been minimized.

Member

humitos commented Feb 1, 2018

@agjohnson @davidfischer I finally was able to combine everything in just one list (in the UX and in the Javascript code). Everything is working now and we have visual feedback.

I noted a couple of things that are missing in the first comment and I will need help no those (something about knockout and some CSS).

@humitos

This comment has been minimized.

Member

humitos commented Feb 15, 2018

I just rebased this branch to fix the conflicts that it had. It needs more work on the items listed in the first comments but I don't know how to do them.

humitos added some commits Feb 15, 2018

Show all connected accounts
Ability to filter by each connected account independently.

humitos and others added some commits Feb 1, 2018

@agjohnson

This comment has been minimized.

Contributor

agjohnson commented Mar 9, 2018

I poked this and it looks great! I rebased things, reran linting on the files, and fixed the LESS and HTML templates. I left the old remote-org rules in for now, in case it breaks the rtd.com.

The last commit of mine is the generated files, view d7fd112...05f8e43 for my changes

+1 on merging if my changes look good!

@agjohnson agjohnson merged commit 8b0350f into master Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agjohnson agjohnson deleted the humitos/import/filter-by-own-repos branch Mar 9, 2018

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