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

Filter by my own repositories at Import Remote Project #3548

Merged
merged 16 commits into from Mar 9, 2018

Conversation

@humitos
Copy link
Member

@humitos 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)
Copy link
Member

@safwanrahman safwanrahman Jan 25, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@humitos humitos left a comment

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.

Loading

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

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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.

Loading

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?
Copy link
Member Author

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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.

Loading

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>
Copy link
Member Author

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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?

Loading

Copy link
Member Author

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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

Loading

@humitos
Copy link
Member Author

@humitos 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.

Loading

@humitos
Copy link
Member Author

@humitos humitos commented Jan 25, 2018

Example,

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

Loading

@@ -157,14 +157,23 @@ function ProjectImportView (instance, config) {

ko.computed(function () {
var org = self.filter_org(),
orgs = self.organizations(),
Copy link
Member Author

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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

Loading

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

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@humitos humitos Jan 30, 2018

Choose a reason for hiding this comment

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

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.

Loading

<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>
Copy link
Member Author

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@humitos humitos Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

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

@humitos humitos Jan 25, 2018

Choose a reason for hiding this comment

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

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?

Loading

Copy link
Contributor

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@humitos humitos Jan 26, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@humitos humitos Jan 30, 2018

Choose a reason for hiding this comment

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

I created a new endpoint at a9d98a2

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

Loading

{# 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>
Copy link
Contributor

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor

@davidfischer davidfischer Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@humitos humitos Feb 1, 2018

Choose a reason for hiding this comment

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

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 :)

Loading

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

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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

Loading

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

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

<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>
Copy link
Contributor

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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

Loading

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

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member Author

@humitos humitos Jan 26, 2018

Choose a reason for hiding this comment

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

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

Loading

self.filter_org(id);
self.filter_own(false);
Copy link
Contributor

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member Author

@humitos humitos Feb 1, 2018

Choose a reason for hiding this comment

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

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.

Loading


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

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

Same as above here.

Loading

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

@agjohnson agjohnson Jan 26, 2018

Choose a reason for hiding this comment

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

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.

Loading

@davidfischer
Copy link
Contributor

@davidfischer 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.

Loading

@humitos
Copy link
Member Author

@humitos 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).

Loading

@humitos humitos force-pushed the humitos/import/filter-by-own-repos branch from b293472 to 6952e72 Feb 15, 2018
@humitos
Copy link
Member Author

@humitos 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.

Loading

@agjohnson agjohnson force-pushed the humitos/import/filter-by-own-repos branch from 6952e72 to 0955072 Mar 9, 2018
@agjohnson
Copy link
Contributor

@agjohnson 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!

Loading

@agjohnson agjohnson merged commit 8b0350f into master Mar 9, 2018
1 check passed
Loading
@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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants