Feature/update slug #2868

saransh-dev · 2017-08-31T13:11:09Z

Fixed issue #2805

Closes: #2805

…ferent line

marla-singer

PR has new files as .DS_Store.
If it is auto-generated files of your Editor or OS then add a rule for .gitignore

marla-singer · 2017-09-01T08:42:59Z

apinf_packages/apis/client/profile/settings/details/autoform.js

+        FlowRouter.go('viewApi', { slug });
+      } else {
+        // Otherwise Redirect to API Catalog
+        FlowRouter.go('apiCatalog');


There is incorrect logic.
If a user will rename an API it would redirect to API Profile again
If a user will edit just URL it would redirect to API catalog

My suggestion (as I wrote in another PR) :

AutoForm.hooks({ apiDetailsForm: { onSuccess () { const slug = this.updateDoc.$set.slug; // make sure slug was edited if (slug) { // Redirect to updated API with new slug FlowRouter.go('viewApi', { slug }); } // Get success message translation const message = TAPi18n.__('apiDetailsForm_text_updateInformation'); // Show message sAlert.success(message); }, }, });

if it return this.updateDoc.$set.slug undefined then code will stuck.

then code will stuck.

I tested it local and everything is fine.

ok i'll remove it

marla-singer · 2017-09-01T08:44:23Z

apinf_packages/apis/client/profile/view.js

-
-  // Subscribe to public proxy details for proxy form
-  instance.subscribe('publicProxyDetails');
+  const templateInstance = this;


Why does it need two variable for this?
Remove templateInstance variable and use instance for that

@marla-singer we use the variable templateInstance to refer to the Template instance. This is following conventions from our lint rules.

See, for example, template event handler arguments:

platform/apinf_packages/api_filter/client/filter_icon.js

Line 10 in 38bc28a

'click #filter-icon': (event, templateInstance) => {

platform/apinf_packages/proxy_backends/client/modal_form/change_selected_proxy/change_selected_proxy.js

Line 14 in 38bc28a

'click #confirm-change-selected-proxy': function (event, templateInstance) {

platform/apinf_packages/apis/client/delete/delete.js

Line 17 in 38bc28a

'click #modal-delete-api': function (event, templateInstance) {

platform/apinf_packages/apis_authorization/client/remove/remove.js

Line 16 in 38bc28a

'click #confirm-remove': function (event, templateInstance) {

platform/apinf_packages/organizations/client/managers/remove/remove.js

Line 14 in 38bc28a

'click #confirm-remove': function (event, templateInstance) {

platform/apinf_packages/apis/client/export/export.js

Line 19 in 38bc28a

'click #exportJSONConfig': function (event, templateInstance) {

platform/apinf_packages/related_media/client/delete/delete_modal.js

Line 11 in 38bc28a

'click #modal-delete-post': function (event, templateInstance) {

Etc...
https://github.com/apinf/platform/search?utf8=✓&q=templateInstance

@brylie Then it should be resolved in another PR in a whole project
I don't mind to rename the variable.

Yes, we should definitely be consistent with the templateInstance variable. For this PR, lets go ahead and prefer the templateInstance naming convention. The clean-up work for other files can take place, as you mention, in a separate PR.

we can use templateInstance instead on instance

@saransh-dev Remove const instance = this and rename all instance to templateInstance in current file

marla-singer · 2017-09-01T08:49:28Z

apinf_packages/apis/client/profile/view.js

+  templateInstance.autorun(() => {
+    // Take slug from params
+    const slug = FlowRouter.getParam('slug');
+    instance.slug = slug;


Why do you duplication value of slug?
I saw that you didn't use instance.slug in helpers.

Keep variable const slug and remove instance.slug

ok i'll do this

marla-singer · 2017-09-01T08:50:04Z

apinf_packages/apis/client/profile/view.js

+    // Subscribe to public proxy details for proxy form
+    instance.subscribe('publicProxyDetails');
+    if (instance.subscriptionsReady()) {
+      // Get single API Backend


Remove condition if (instance.subscriptionsReady()) {

marla-singer · 2017-09-01T08:59:27Z

@saransh-dev Also don't send a review request to no developer team member. The PR can be merged by myself, Brylie (@brylie), Matti (@matleppa ) and Mauricio (@mauriciovieira)

mauriciovieira

Can you squash all 'Delete .DS_Store' commits into a single one?

… into feature/update-slug

marla-singer · 2017-09-07T11:55:44Z

@saransh-dev Resolve conflict

brylie · 2017-09-07T12:18:39Z

@saransh-dev It would probably just be better to cherry-pick your commits in a new branch from develop, rather than continuing to work in this branch.

saransh-dev · 2017-09-08T05:16:40Z

@marla-singer Above conflict occur because of following:-

1) 03 user creation create admin user:

     Uncaught Error: unknown error: Element is not clickable at point (392, 767)

      at SettingsPage.submit (tests/page-objects/settings.page.js:50:29)

      at SettingsPage.setupGithub (tests/page-objects/settings.page.js:45:10)

      at Context.<anonymous> (tests/end-to-end/ui/03-user-creation.js:39:18)

      at node_modules/chimp/dist/lib/utils/fiberize.js:29:22

I have also attached screen shot for more reference.

marla-singer · 2017-09-08T08:09:44Z

@saransh-dev

you've got remarks from another reviewer
YourPR conflicts with develop branch and it doesn't relate to test fails

marla-singer · 2017-09-08T09:26:43Z

@saransh-dev Make git rebase develop - it allows to set your commits to up
https://git-scm.com/docs/git-rebase

… into feature/update-slug

Update based on a new translation round

Update Finnish translations

… into feature/update-slug

mauriciovieira · 2017-09-25T17:01:01Z

@saransh-dev May I take over this branch and do the rebase?

saransh-dev · 2017-09-26T04:54:32Z

@mauriciovieira , Yes, you can rebase it.

mauriciovieira · 2017-09-27T14:52:27Z

@marla-singer @saransh-dev I squashed all @saransh-dev commits and added a final one in #2991.

saransh1510 added 13 commits August 22, 2017 12:43

fix issue slug not change after update api name

7930edd

fix issue slug not change after update api name

68869db

fix issue slug not change after update api name

69549b0

fix issue slug not change after update api name

4e0a49a

change in comment

28c988c

fix error error Line 18 exceeds the maximum line length of 100 max-len

15bb240

remove exception while update api name

80b6830

remove conflicts

db9ac68

resolve conflicts

8a06916

remove error of spacing after if

5649f54

fix errors like Missing space before =>

d936795

fix errors like Block must not be padded by blank lines

434d87c

remove error of Trailing spaces not allowed no-trailing-spaces

feab7b3

saransh-dev added the Ready for review label Aug 31, 2017

saransh-dev requested review from brylie, saralavanip and marla-singer August 31, 2017 13:11

saransh1510 added 7 commits August 31, 2017 18:50

remove error of Block must not be padded by blank lines

02d33ea

remove error of Trailing spaces not allowed no-trailing-spaces

b48e398

remove error of Trailing spaces not allowed no-trailing-spaces on dif…

f5b835f

…ferent line

remove error of undefined

8a6f8d7

resolve error of ReactiveVar is not defined no-undef

8ff7a42

remveo errors

05e3662

remove conflicts

051e1f9

marla-singer suggested changes Sep 1, 2017

View reviewed changes

marla-singer removed the request for review from saralavanip September 1, 2017 08:51

marla-singer added this to the Sprint 51 milestone Sep 1, 2017

marla-singer self-assigned this Sep 1, 2017

marla-singer removed the Ready for review label Sep 1, 2017

saransh1510 added 4 commits September 5, 2017 18:06

Delete .DS_Store

b236b0b

Delete .DS_Store

af8e95f

Delete .DS_Store

acf8af8

Delete .DS_Store

aea62ec

mauriciovieira suggested changes Sep 5, 2017

View reviewed changes

Merge branch 'feature/update-slug' of https://github.com/apinf/platform…

e4f1576

… into feature/update-slug

bajiat added the in progress label Sep 6, 2017

remove conflict

b90569c

saransh1510 and others added 8 commits September 13, 2017 13:05

Merge branch 'feature/update-slug' of https://github.com/apinf/platform…

3907fe5

… into feature/update-slug

Update Finnish translations

1b8fa72

Update based on a new translation round

Update fi.i18n.json

da23764

Merge branch 'develop' into feature/update-slug

d06e556

Merge pull request #2914 from apinf/feature-update-finnish-translations

5391a51

Update Finnish translations

Update for release 0.50.0

5beff72

Update for release 0.50.0

80022a8

Merge branch 'feature/update-slug' of https://github.com/apinf/platform…

e992b40

… into feature/update-slug

saransh-dev added Ready for review and removed in progress labels Sep 21, 2017

mauriciovieira mentioned this pull request Sep 27, 2017

Chore/update slug #2991

Merged

mauriciovieira closed this Sep 27, 2017

marla-singer deleted the feature/update-slug branch November 13, 2017 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/update slug #2868

Feature/update slug #2868

saransh-dev commented Aug 31, 2017

marla-singer left a comment

marla-singer Sep 1, 2017

saransh-dev Sep 1, 2017

marla-singer Sep 1, 2017 •

edited

Loading

saransh-dev Sep 1, 2017

marla-singer Sep 1, 2017

brylie Sep 1, 2017

marla-singer Sep 1, 2017

brylie Sep 1, 2017

saransh-dev Sep 1, 2017

marla-singer Sep 1, 2017

marla-singer Sep 1, 2017

saransh-dev Sep 1, 2017

marla-singer Sep 1, 2017

marla-singer commented Sep 1, 2017

mauriciovieira left a comment •

edited

Loading

marla-singer commented Sep 7, 2017

brylie commented Sep 7, 2017

saransh-dev commented Sep 8, 2017

marla-singer commented Sep 8, 2017

marla-singer commented Sep 8, 2017

mauriciovieira commented Sep 25, 2017

saransh-dev commented Sep 26, 2017

mauriciovieira commented Sep 27, 2017

Feature/update slug #2868

Feature/update slug #2868

Conversation

saransh-dev commented Aug 31, 2017

Fixed issue #2805

marla-singer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marla-singer Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marla-singer commented Sep 1, 2017

mauriciovieira left a comment • edited Loading

Choose a reason for hiding this comment

marla-singer commented Sep 7, 2017

brylie commented Sep 7, 2017

saransh-dev commented Sep 8, 2017

marla-singer commented Sep 8, 2017

marla-singer commented Sep 8, 2017

mauriciovieira commented Sep 25, 2017

saransh-dev commented Sep 26, 2017

mauriciovieira commented Sep 27, 2017

marla-singer Sep 1, 2017 •

edited

Loading

mauriciovieira left a comment •

edited

Loading