Made URL of new collection page generic #4890

shruti862 · 2025-02-01T17:15:07Z

Summary

Made the URL of new collection page more generic i.e. (/collections/new)

References

Reviewer guidance

…

akolson · 2025-02-11T13:09:17Z

Hi @shruti862! Thank you for the great work so far. @bjester provided some guidance to your question. Also, be sure to let us know incase you have any other questions.

Optional, but you could mark your pr as draft if still actively working in it and open it once it's ready for a review. Or alternatively, whenever you are done, give us a head-up so we can checkout your changes.

Thanks for working on this issue.

shruti862 · 2025-02-11T18:42:32Z

Hey @akolson, I’ve already gone through the guidance provided by @bjester. However, since my midsem exams are currently ongoing, I haven’t had time to work on it. I’ll get back to this issue in 2-3 days. Until then, as you suggested, I’m marking this PR as a draft.

akolson · 2025-02-11T19:07:33Z

Thanks @shruti862 for the heads-up, and no pressure at all. Best wishes in your exams. See you soon! 👋

shruti862 · 2025-02-18T11:48:09Z

Hey @akolson @bjester , The PR is ready for review :) please have a look whenever you get time .

akolson · 2025-02-19T14:50:34Z

contentcuration/contentcuration/frontend/channelList/constants.js

@@ -21,6 +21,7 @@ export const RouteNames = {
  CHANNEL_EDIT: 'CHANNEL_EDIT',
  CHANNEL_SETS: 'CHANNEL_SETS',
  CHANNEL_SET_DETAILS: 'CHANNEL_SET_DETAILS',
+  NEW_CHANNEL_SET_DETAILS: 'NEW_CHANNEL_SET_DETAILS',


Can we rename this to NEW_CHANNEL_SET instead for clarity.

NEW_CHANNEL_SET : 'NEW_CHANNEL_SET'

akolson · 2025-02-19T15:02:58Z

contentcuration/contentcuration/frontend/channelList/vuex/channelSet/actions.js

+  return ChannelSet.createModel(channelSetData)
+    .then(data => {
+      if (!data || !data.id) {
+        console.error('Error: The created channel set does not have an ID', data);


akolson · 2025-02-19T15:04:21Z

contentcuration/contentcuration/frontend/channelList/vuex/channelSet/actions.js

+    .then(data => {
+      if (!data || !data.id) {
+        console.error('Error: The created channel set does not have an ID', data);
+        throw new Error('The created channel set does not have an ID');


I think we can throw a less generic error here. We could reuse ReferenceError?

akolson · 2025-02-19T15:08:55Z

contentcuration/contentcuration/frontend/channelList/vuex/channelSet/actions.js

+      return data;
+    })
+    .catch(error => {
+      console.error('Error creating channel set:', error);


akolson · 2025-02-19T15:38:32Z

contentcuration/contentcuration/frontend/channelList/views/ChannelSet/ChannelSetModal.vue

-      return this.verifyChannelSet(this.channelSetId);
+      if (this.channelSetId) {
+        return this.verifyChannelSet(this.channelSetId);
+      } else this.setup();


Can we keep the syntax consistent here?

if (this.channelSetId) { return this.verifyChannelSet(this.channelSetId); } else { this.setup(); }

Also, is there any reason we are returning this.verifyChannelSet?

Hey @akolson
this.verifyChannelSet is invoked for existing collections in the edit functionality to verify the existence of the associated channelSetId.

@shruti862 I see. What would be the significance in returning its value to the beforeMount() hook?

shruti862 · 2025-02-19T18:57:53Z

Hey @akolson , I have done all the suggested changes :)

akolson · 2025-02-21T06:15:28Z

contentcuration/contentcuration/frontend/channelList/views/ChannelSet/ChannelSetModal.vue

@@ -199,7 +202,7 @@
    computed: {
      ...mapGetters('channelSet', ['getChannelSet']),
      isNew() {
-        return Boolean(this.channelSet[NEW_OBJECT]);
+        return !this.channelSetId || this.$route.path === '/collections/new';


Are there scenarios where channelSetId is empty(not set) and we are not creating a new collection?

akolson · 2025-02-21T06:20:29Z

contentcuration/contentcuration/frontend/channelList/views/ChannelSet/ChannelSetModal.vue

+                return newCollection;
+              })
+              .catch(error => {
+                console.error('Error while creating collection:', error);


akolson · 2025-02-21T06:25:15Z

contentcuration/contentcuration/frontend/channelList/views/ChannelSet/ChannelSetModal.vue

-            resolve();
-            return;
-          }
+        if (!channelSetId || channelSetId === 'new') {


I can't seem to see where channelSetId is being set to new for the or part of this condition to be successful?

akolson · 2025-02-21T06:40:47Z

contentcuration/contentcuration/frontend/channelList/views/ChannelSet/ChannelSetModal.vue

-            reject();
+          })
+          .catch(error => {
+            console.error('Error loading collection:', error);


With the loadChannelSet error being suppressed, I don't think this will be useful

studio/contentcuration/contentcuration/frontend/channelList/vuex/channelSet/actions.js

Lines 12 to 21 in dbb5a63

export function loadChannelSet(context, id) {

return ChannelSet.get(id)

.then(channelSet => {

context.commit('ADD_CHANNELSET', channelSet);

return channelSet;

})

.catch(() => {

return;

});

}

akolson · 2025-02-21T06:53:33Z

contentcuration/contentcuration/frontend/channelList/views/ChannelSet/ChannelSetModal.vue

          });
-        });
      },
    },


It is generally not clear to me why we are returning a promise in some parts of this method i.e the lines with return Promise.resolve();. Is there a motivation behind this? It doesn't look like we need to return them.

Hey @akolson , It seems that there is no need to change verifyChannelSet method so I just replaced it with older version.

akolson

@shruti862 great work, we are surely getting closer! I left a few additional comments and some clarifying questions. Thanks again.

shruti862 · 2025-02-21T16:08:45Z

Hey @akolson, thanks for your great review! I’ve made some changes to the code to accommodate your suggestions. Could you please take a look when you have a moment?

akolson

Looking good. I also did a bit of manual testing and everything seems to work just fine, also resolves the bug reported. Thank you @shruti862!

shruti862 and others added 4 commits February 1, 2025 15:11

new commits

c34b61f

new changes

8fb902a

Merge branch 'learningequality:unstable' into fix-2450

0d4453f

[pre-commit.ci lite] apply automatic fixes

14dde08

MisRob requested review from akolson and nucleogenesis February 5, 2025 15:21

MisRob added the TODO: needs review label Feb 5, 2025

MisRob requested a review from AlexVelezLl February 5, 2025 15:23

akolson self-assigned this Feb 11, 2025

shruti862 marked this pull request as draft February 11, 2025 18:43

shruti862 and others added 3 commits February 16, 2025 15:49

Merge branch 'learningequality:unstable' into fix-2450

1fa1bb1

new commits

6f10821

[pre-commit.ci lite] apply automatic fixes

e29d3d3

shruti862 marked this pull request as ready for review February 18, 2025 11:48

shruti862 and others added 6 commits February 18, 2025 18:43

fixed failing frontend test channelSetList.spec.js

cdaec53

Merge branch 'fix-2450' of github.com:shruti862/studio into fix-2450

9ce0d63

[pre-commit.ci lite] apply automatic fixes

274c296

changed channelSetModal.spec.js test

c4a84cb

Merge branch 'fix-2450' of github.com:shruti862/studio into fix-2450

2869650

[pre-commit.ci lite] apply automatic fixes

955ce06

akolson reviewed Feb 19, 2025

View reviewed changes

done suggested changes

02b9e52

shruti862 and others added 2 commits February 20, 2025 00:23

Merge branch 'fix-2450' of github.com:shruti862/studio into fix-2450

0401583

[pre-commit.ci lite] apply automatic fixes

db1e707

shruti862 added 2 commits February 20, 2025 00:33

new commit

e0f4272

Merge branch 'fix-2450' of github.com:shruti862/studio into fix-2450

f5b3f51

akolson reviewed Feb 21, 2025

View reviewed changes

akolson requested changes Feb 21, 2025

View reviewed changes

shruti862 and others added 2 commits February 21, 2025 21:32

new changes done

c8983fe

[pre-commit.ci lite] apply automatic fixes

c0d8164

akolson self-requested a review February 21, 2025 18:22

akolson approved these changes Feb 24, 2025

View reviewed changes

akolson merged commit 184cfbf into learningequality:unstable Feb 24, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made URL of new collection page generic #4890

Made URL of new collection page generic #4890

shruti862 commented Feb 1, 2025 •

edited by akolson

Loading

akolson commented Feb 11, 2025

shruti862 commented Feb 11, 2025

akolson commented Feb 11, 2025

shruti862 commented Feb 18, 2025

akolson Feb 19, 2025

akolson Feb 19, 2025

akolson Feb 19, 2025

akolson Feb 19, 2025

akolson Feb 19, 2025

akolson Feb 19, 2025

shruti862 Feb 19, 2025

akolson Feb 21, 2025 •

edited

Loading

shruti862 commented Feb 19, 2025

akolson Feb 21, 2025

akolson Feb 21, 2025

akolson Feb 21, 2025 •

edited

Loading

akolson Feb 21, 2025

akolson Feb 21, 2025

shruti862 Feb 21, 2025

akolson left a comment

shruti862 commented Feb 21, 2025

akolson left a comment

	export function loadChannelSet(context, id) {
	return ChannelSet.get(id)
	.then(channelSet => {
	context.commit('ADD_CHANNELSET', channelSet);
	return channelSet;
	})
	.catch(() => {
	return;
	});
	}

Made URL of new collection page generic #4890

Made URL of new collection page generic #4890

Conversation

shruti862 commented Feb 1, 2025 • edited by akolson Loading

Summary

References

Reviewer guidance

akolson commented Feb 11, 2025

shruti862 commented Feb 11, 2025

akolson commented Feb 11, 2025

shruti862 commented Feb 18, 2025

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

akolson Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

shruti862 commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson Feb 21, 2025 • 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

akolson left a comment

Choose a reason for hiding this comment

shruti862 commented Feb 21, 2025

akolson left a comment

Choose a reason for hiding this comment

shruti862 commented Feb 1, 2025 •

edited by akolson

Loading

akolson Feb 21, 2025 •

edited

Loading

akolson Feb 21, 2025 •

edited

Loading